Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue with sendOptions.maxAge being overwritten #24

Merged
merged 4 commits into from
Nov 22, 2018

Conversation

ceecko
Copy link
Contributor

@ceecko ceecko commented Nov 17, 2018

This fix resolves issue with sendOptions.maxAge being overwritten when 404 is returned.
The following scenario resets the maxAge property:

  1. Load static file which exists (maxAge respects configured sendOptions)
  2. Load static file which does not exist (maxAge is set to 0)
  3. Load static file which exists (maxAge is set still to 0 and MAX_AGE is used instead)

@XhmikosR
Copy link
Collaborator

@ceecko: if you add a test for this, we can merge it.

@ceecko
Copy link
Contributor Author

ceecko commented Nov 18, 2018

@XhmikosR I have added tests and a new option maxAgeNonHashed which specifies maxAge for non-hashed assets to add more flexibility.

@@ -173,7 +177,7 @@ describe('.serve', () => {

it('should serve files without a hash tag', done => {
http.get('http://localhost:12321/index.js', res => {
res.headers['cache-control'].includes('max-age=0').should.be.true();
res.headers['cache-control'].includes('max-age=7200').should.be.true();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still like to have a test for max-age=0, which is the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests on lines 97 and 136 already check max-age=0
This test suite is meant to check custom max-age for non-hashed assets

@XhmikosR
Copy link
Collaborator

Also, please fetch upstream and rebase. :)

@XhmikosR
Copy link
Collaborator

XhmikosR commented Nov 19, 2018

That's a merge not a rebase BTW. So it's unmergeable.

@XhmikosR
Copy link
Collaborator

git rebase -i upstream/master and fix your commits please.

@ceecko
Copy link
Contributor Author

ceecko commented Nov 19, 2018

Yeah, sorry. Messed up a bit.
Now it should be fine.

@XhmikosR
Copy link
Collaborator

Thanks, I'm gonna have a look tomorrow.

sendOptions: opts.sendOptions || {}
};

defaultOptions = Object.assign(defaultOptions, opts);

defaultOptions.sendOptions.root = root;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we move these 2 into setOptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, makes sense

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still feel like the following assignments aren't all needed.

We just have one options Object. Can't you put the new options there and just base your logic on maxAge?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there was only one options object, the logic in serve would have to change maxAge for non-hashed assets and after calling send reset maxAge back to original value which may be confusing and introduces unnecessary overhead with every call.

The reason for this pull request is to resolve issues with a single options object's maxAge being overwritten when a non-hashed asset is requested. Such call resets maxAge to 0 and all subsequent requests started to use MAX_AGE.

@XhmikosR XhmikosR merged commit fd846c2 into errorception:master Nov 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants