-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Conversation
Appears the tests are failing, could you fix that? |
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! |
Sorry if my above comment wasn't clear - Do you want me to try and fix these tests which are broken on master? |
@Levertion if you want to yes. Probably best to do it in a seperate pr Sent with GitHawk |
Sorry. I meant as a blocker on merging this! |
@Levertion it would be helpfull in reviewing if this breaks anything but it’s not a blocker no Sent with GitHawk |
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 intosrc\index.js
on windows, giving the identifier"2gen"
.On other platforms, where the platform seperator is
/
,Asset::relativeName
would besrc/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
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 failuresAdditionally, I am not sure how I would actually test this behaviour, given that it uses the platform specific
path
module directly.