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

Make builds deterministic #780

Closed
wants to merge 4 commits into from
Closed

Make builds deterministic #780

wants to merge 4 commits into from

Conversation

fu5ha
Copy link
Contributor

@fu5ha fu5ha commented Feb 10, 2018

Fix/improvement based on #735

Makes asset ids the last 5 characters of the md5 hash of the path of that asset.

@fu5ha
Copy link
Contributor Author

fu5ha commented Feb 10, 2018

wip... still looking to improve this and pass all the tests

@codecov-io
Copy link

codecov-io commented Feb 10, 2018

Codecov Report

Merging #780 into master will decrease coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #780      +/-   ##
==========================================
- Coverage   94.02%   93.88%   -0.14%     
==========================================
  Files          65       65              
  Lines        2174     2175       +1     
==========================================
- Hits         2044     2042       -2     
- Misses        130      133       +3
Impacted Files Coverage Δ
src/Asset.js 97.75% <100%> (+0.02%) ⬆️
src/assets/CSSAsset.js 82.35% <0%> (-7.36%) ⬇️
src/Logger.js 96.96% <0%> (+1.51%) ⬆️
src/WorkerFarm.js 93.22% <0%> (+1.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e224bd...19375df. Read the comment docs.

@fu5ha
Copy link
Contributor Author

fu5ha commented Feb 10, 2018

Alright this passes tests now. Main downside is that it could increase minified file sizes a small amount and it may very slightly affect performance, maybe noticeable for projects with a very large number of assets.

@jeanfortheweb
Copy link

I had a similar approach a few days ago, but I used the Asset name itself since it's already the absolute asset path itself which should be perfect for unique ID's. Anyway, I also discovered that assets are forced to be unique created inside the bundler, therefore the current ID generation should already deterministic as long as the assets do not change. If that's not the case, then the files are not getting read in the same order every time.

At least I would think about moving the asset creation from Bundler.js:259 (on 1.4.1) to the Asset itself using a factory pattern. Guess that would make it a lot easier to reason about.

@fu5ha
Copy link
Contributor Author

fu5ha commented Feb 12, 2018

That's what I did originally, but they change across different machines/operating systems. Using a normalized relative path seems to be the best options as far as that goes. See #735 for discussion/my findings about why this isn't happening--ultimately I reached a similar conclusion to you. Something's going on where assets are being loaded multiple times and not always in the same order which is why the current system doesn't work properly.. possibly because of the way workers are used to parallelize the work.

@jeanfortheweb
Copy link

jeanfortheweb commented Feb 12, 2018

Just a guess but it shouldn't happen at load time, since this is happening in the main thread on the bundler... hmmm, well if the loading order depending on the native file system layer can't be deterministic then these IDs will also never be. But I like the simplicity of numeric IDs, actually.

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