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

reset etag with pathname only #1418

Merged
merged 3 commits into from
Oct 29, 2020
Merged

Conversation

dishuostec
Copy link
Contributor

try to fix #1409

Changes

When running dev, if requesting url contains search part, then set etag to both origin url and pathname.

Testing

Don't know how to simply test response code for dev mode.

Docs

bug fix only

@vercel
Copy link

vercel bot commented Oct 27, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/cpj6n7rq8
✅ Preview: https://snowpack-git-fix-etag.pikapkg.vercel.app

const tag = etag(result.contents, {weak: true});
knownETags.set(reqPath, tag);
if (reqPath.indexOf('?') >= 0) {
knownETags.set(reqPath.split('?', 2)[0], tag);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems fine to me, but @FredKSchott would know more if this affects anything for the etag.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah yea, good catch. We have another way of doing this in the rest of the codebase: const reqPath = decodeURI(url.parse(reqUrl).pathname!); I'll update this PR to use that method.

@FredKSchott FredKSchott merged commit 7a42bdd into FredKSchott:master Oct 29, 2020
@dishuostec dishuostec deleted the fix-etag branch October 30, 2020 00:48
peterhpchen pushed a commit to peterhpchen/snowpack that referenced this pull request Oct 31, 2020
* reset etag with pathname only

* Update dev.ts

* Update dev.ts

Co-authored-by: Fred K. Schott <fkschott@gmail.com>
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.

etag is not correctly update
3 participants