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

use a simpler hashing algorithm when a file system is available #112

Merged
merged 3 commits into from
Dec 22, 2017

Conversation

quantizor
Copy link
Collaborator

fixes #105

By not including the file contents in the hash, the generated IDs
should be more durable and less susceptible to minor differences
in file content due to a babel plugin being applied to ssr vs
client config or something like that

@quantizor quantizor force-pushed the es-hash-adjustment branch 2 times, most recently from c1173a2 to 44ee2b0 Compare December 20, 2017 18:21
@quantizor
Copy link
Collaborator Author

quantizor commented Dec 20, 2017

Going to add a snapshot serializer to do the whitespace removal stuff done in
8037d30

@quantizor quantizor force-pushed the es-hash-adjustment branch 2 times, most recently from a9ebe59 to 77bff83 Compare December 20, 2017 19:00
Copy link
Member

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

This looks good in terms of the code that changed, but the snapshot change is making it very hard to review what exactly this affects/if it works. 😕

@quantizor
Copy link
Collaborator Author

quantizor commented Dec 20, 2017

@mxstbr even with the separate commits? I'll merge the whitespace serializer back into the first two commits to make it as clear as possible

@quantizor
Copy link
Collaborator Author

quantizor commented Dec 20, 2017

@mxstbr I'll split up 8037d30 such that the rename from before.js to index.js happens in a separate commit. That way you'll be able to verify that the original snapshots are accurately recreated.

I ended up dropping the rename from before.js to index.js so now in 2445cca it should be clear that the hashes did not change

@quantizor
Copy link
Collaborator Author

There we go.

Copy link
Member

@mxstbr mxstbr left a comment

Choose a reason for hiding this comment

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

LGTM, @philpl? Any reasons why we shouldn't do it this way?

@mxstbr
Copy link
Member

mxstbr commented Dec 22, 2017

Shipping this and the ref styled-components PR! styled-components/styled-components#1381

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.

File hash in displayName is too fragile
2 participants