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

Cross-platform deterministic asset ids #2020

Merged
merged 2 commits into from
Sep 22, 2018

Conversation

Levertion
Copy link
Contributor

@Levertion Levertion commented Sep 16, 2018

Makes the deterministic asset ids introduced in #1694 not dependant on platform.

On windows, path.relative uses \, whereas on other systems it uses /. Because of this, the hashes were different.

💻 Examples

For a file at <root>/src/index.js, Asset::relativeName would be transformed into src\index.js on windows, giving the identifier "2gen".

On other platforms, where the platform seperator is /, Asset::relativeName would be src/index.js as expected, with the identifier "H99C".

This is causing unwanted build failures for my project when dist is committed to master, which I am doing to allow my typescript project to be used as an npm module directly from github. See Levertion/mcfunction-langserver#69

✔️ PR Todo

  • Added/updated unit tests for this change

I haven't added any tests as #1694 did not change any tests and was included in a patch release => asset identifiers are not guaranteed to be backwards compatible.

The build fails on CI, but I think that is not related to this change as master has the same failures

Additionally, I am not sure how I would actually test this behaviour, given that it uses the platform specific path module directly.

Levertion added a commit to Levertion/mcfunction-langserver that referenced this pull request Sep 16, 2018
@DeMoorJasper
Copy link
Member

Appears the tests are failing, could you fix that?

@Levertion
Copy link
Contributor Author

I compared the test failures on master https://travis-ci.org/parcel-bundler/parcel/builds/429153627 to mine https://travis-ci.org/parcel-bundler/parcel/jobs/429243614, and they were all the same failures. This suggested that the failures were spurious, as I outlined above. If, however, the github settings are such that statuses much pass to merge into master (which I doubt as ffacdf8 was a direct commit), then I'll try to fix as many as I can.

Tl;dr I don't know how to fix all 37 broken tests on master!

@Levertion
Copy link
Contributor Author

Sorry if my above comment wasn't clear - Do you want me to try and fix these tests which are broken on master?

@DeMoorJasper
Copy link
Member

@Levertion if you want to yes. Probably best to do it in a seperate pr

Sent with GitHawk

@Levertion
Copy link
Contributor Author

Sorry. I meant as a blocker on merging this!

@DeMoorJasper
Copy link
Member

@Levertion it would be helpfull in reviewing if this breaks anything but it’s not a blocker no

Sent with GitHawk

DeMoorJasper
DeMoorJasper approved these changes Sep 20, 2018
@devongovett devongovett merged commit def6f1e into parcel-bundler:master Sep 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.

3 participants