fixes #933 : JSPackager deduplication now accounts for differences in absolute dependency paths #1011

pcattori · 2018-03-17T03:49:43Z

codecov-io · 2018-03-17T04:01:39Z

Codecov Report

Merging #1011 into master will increase coverage by 2.17%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1011      +/-   ##
==========================================
+ Coverage   86.35%   88.53%   +2.17%     
==========================================
  Files          68       68              
  Lines        3541     3366     -175     
==========================================
- Hits         3058     2980      -78     
+ Misses        483      386      -97

Impacted Files	Coverage Δ
src/packagers/JSPackager.js	`90.38% <100%> (+0.38%)`	⬆️
src/assets/ReasonAsset.js	`91.66% <0%> (-8.34%)`	⬇️
src/visitors/dependencies.js	`77.92% <0%> (-7.8%)`	⬇️
src/transforms/babel.js	`87.86% <0%> (-7.77%)`	⬇️
src/assets/HTMLAsset.js	`85.1% <0%> (-2.13%)`	⬇️
src/assets/JSAsset.js	`90.83% <0%> (-0.77%)`	⬇️
src/assets/CoffeeScriptAsset.js	`100% <0%> (ø)`	⬆️
src/utils/syncPromise.js	`100% <0%> (ø)`	⬆️
src/assets/TOMLAsset.js	`100% <0%> (ø)`	⬆️
src/assets/GraphqlAsset.js	`100% <0%> (ø)`	⬆️
... and 14 more

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 fc654d7...0e38d87. Read the comment docs.

devongovett · 2018-03-18T03:16:40Z

src/packagers/JSPackager.js

+const dedupKey = asset => {
+  // cannot rely only on generated JS for deduplication as paths like `../` can
+  // cause 2 identical JS files to be different depending on filesystem locations
+  return {


We cannot use objects as keys for the dedupe map - they will never match since JS compares keys by object identity. You'll need to generate a string key.

pcattori · 2018-03-19T18:16:37Z

@devongovett addressed your review comments and refactored to use Resolver.resolveFilename 😄

devongovett

Would be awesome if you could add a test. Let me know if you would like any help with that!

devongovett · 2018-03-28T04:42:58Z

src/packagers/JSPackager.js

+    // cannot rely *only* on generated JS for deduplication because paths like
+    // `../` can cause 2 identical JS files to behave differently depending on
+    // where they are located on the filesystem
+    let deps = Array.from(asset.dependencies.keys()).map(dep =>


Instead of using asset.dependencies here, you could use asset.depAssets which is a list of resolved assets rather than dependencies. So you won't even need to call resolveFilename, it will already have the resolved filename as its name property.

@devongovett I think it still needs resolveFilename as the name property contains only the relative file name.
But something like this should work:

let deps = Array.from(asset.depAssets.keys(), dep => this.bundler.resolver.resolveFilename(dep.name, asset.name) );

@henri-hulski It should probably be ...resolveFilename(dep, asset.name) in your suggestion as dep is a string.

@felixrabe here dep is not a string. I've logged it out and also tested this fix. It works fine.

Ah right, depAssets is a Map, and its key is not a string but a dep (see Bundler.js where it does asset.depAssets.set(...)). Still need to get used to Maps having non-string keys it seems :)

pcattori · 2018-03-29T15:26:47Z

Regarding tests, my plan would be to have the following 3 tests:

2 files with different contents should not be dedupe'd
2 files with same contents AND same dependency paths should be dedupe'd
2 files with same contents BUT with different dependency paths should not be dedupe'd

@devongovett I can definitely make the quick change to use asset.depAssets. For the tests, it may take me some time to figure out how to best write tests for this repo, so if you can help to implement the tests it would greatly speed things up! 🚀 but if not, I'll get around to it when I have some time 😄

henri-hulski · 2018-04-11T14:11:06Z

Fixes also the bug in #1152.

henri-hulski · 2018-04-16T20:07:46Z

Added a test for the scenario described in #933 in PR #1198.

PS.: Deleted previous comment as that was an issue coming from an outdated branch and which is already fixed.

henri-hulski · 2018-04-18T07:58:35Z

@pcattori Are you still planning to finish this PR?

pcattori · 2018-04-18T15:05:14Z

@henri-hulski unfortunately, I have not had time to work on this recently. I do intend on continuing work on this this weekend, ~~though I'm not sure if this work will be irrelevant once 1.8 is released.~~

~~If 1.8 is going to be released soon, I may wait to see if the issue is fixed then.~~

DeMoorJasper · 2018-04-18T15:08:27Z

@pcattori treeshaking will be optional (opt-out or opt-in) so this will still be relevant as far as I know

_{Sent with GitHawk}

henri-hulski · 2018-04-20T21:09:38Z

@pcattori For me the PR looks good. Just switch to asset.depAssets like in my example above and it should be ready.

olizilla · 2018-05-08T16:35:03Z

Bump. The underlying issues with the dedupe logic that this PR fixes are important. I'm seeing module require ids being incorrectly re-used for multiple dependencies that are not the same when using parcel build on master, but are correctly resolved when tested against this PR.

I think also fixes #793

pcattori · 2018-05-08T17:10:20Z

@olizilla @henri-hulski just writing to let you know that I haven't forgotten about this and have some time set aside this week to get this in. I did some work last weekend to get robust tests for this up and almost have those ready.

olizilla · 2018-05-09T08:39:13Z

Another datapoint for this PR. I've tracked down why the existing dedupe logic breaks when bundling IPFS as a dependency; two of it's deps have identical index.js files, as they follow a pattern of requiring the same two files and deferring the differences to other files. See:

See:

https://github.com/ipld/js-ipld-dag-cbor/blob/e892dc987d32bdad9dba0f2d86d27ebc254fe967/src/index.js

and

https://github.com/ipld/js-ipld-git/blob/20f7a66c2fc916aeb4d7e6c7141bfe275033472f/src/index.js

When these two unrelated files are used as keys in the dedupe map, they are the same, so they get deduped, even though they come from different modules. As they are the entry point to a dep, that means the whole require is swapped out for the wrong module.

pcattori · 2018-05-13T02:48:17Z

Alright! This now includes the 3 tests I mentioned before as well as all the suggestions left by reviewers so far.

Note that the 2nd test is the important one here (should not dedupe imports with same content but different absolute dependency paths). This is the test that goes from red to green due to the changes in JSPackager.js (the other 2 tests are just there to make sure those changes didn't break other existing behaviors).

@henri-hulski let me know if it looks good to you 😄

henri-hulski · 2018-05-15T14:31:42Z

@pcattori Looks good to me, but in Windows (AppVeyor) one test is failing:

1) javascript
       should dedupe imports with same content and same dependency paths:
      AssertionError: false == true
      + expected - actual
      -false
      +true
      
      at Context.<anonymous> (test\javascript.js:871:5)
      at undefined.next (native)
      at step (test\javascript.js:3:191)
      at C:\projects\parcel\test\javascript.js:3:361
      at process._tickCallback (internal/process/next_tick.js:109:7)
  
error An unexpected error occurred: "Command failed.
Exit code: 1
Command: C:\\windows\\system32\\cmd.exe
Arguments: /d /s /c cross-env NODE_ENV=test mocha
Directory: C:\\projects\\parcel
Output:
".
info If you think this is a bug, please open a bug report with the information provided in "C:\\projects\\parcel\\yarn-error.log".
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

henri-hulski · 2018-05-15T14:34:55Z

Maybe a problem with slashes vs. backslashes.

pcattori · 2018-05-16T01:08:18Z

Yep, looks like making the paths xplatform did the trick. Now the only tests that are failing are some flakey wasm-related tests (which I think is a known issue?)

henri-hulski

Looks ready to me. Great work!
@devongovett could you review?

olizilla · 2018-05-31T08:36:13Z

It'd be great to get this merged. The "files as Map key" issue causes imports to incorrectly resolve to unrelated modules which happen to have the same shaped index.js file as any other dependency in your tree, which is causing hard to debug errors, and only for production builds.

See ipfs/js-ipfs#1344

pcattori · 2018-06-06T02:54:55Z

@devongovett just a friendly ping 😄

asyncx · 2018-06-12T00:40:13Z

Any idea when this will be merged?

henri-hulski · 2018-06-20T09:26:17Z

Is there a reason why this doesn't get merged?
Parcel is broken for months in nearly all of my parcel projects.
@devongovett I think you can close your change request.

olizilla · 2018-06-26T14:40:11Z

Is it the test failure that's blocking this? It looks like it's unrelated

  314 passing (4m)
  2 failing
  1) rust
       should use wasm-gc to minify output:
     Error: cargo failed.
      at ChildProcess.<anonymous> (src/utils/pipeSpawn.js:1:3757)
      at maybeClose (internal/child_process.js:925:16)
      at Process.ChildProcess._handle.onexit (internal/child_process.js:209:5)
  
  2) wasm
       --target=node
         should load a wasm file in parallel with a dynamic JS import:
     Error: Cannot find module '1678'
      at newRequire (evalmachine.<anonymous>:39:19)
      at newRequire (<anonymous>:31:1)

pcattori · 2018-06-26T15:10:07Z

@olizilla yea definitely unrelated. those tests were sporadically failing before I made my changes as well. like @henri-hulski mentions, i think the only thing blocking this is @devongovett 's change request

gaspard · 2018-07-02T16:02:00Z

Please unlock this PR @devongovett. Your requests for changes seems to be blocking.

devongovett · 2018-07-02T16:49:08Z

src/packagers/JSPackager.js

+    // `../` can cause 2 identical JS files to behave differently depending on
+    // where they are located on the filesystem
+    let deps = Array.from(asset.depAssets.keys(), dep =>
+      this.bundler.resolver.resolveFilename(dep.name, asset.name)


if you use depAssets.values(), you can just map to dep.name which is already resolved

…l build`)

… support

devongovett · 2018-07-02T17:11:13Z

Updated to fix the last comment (use pre-resolved deps), and switched it to use objectHash instead of JSON.stringify (seemed slightly faster in my tests due to shorter string keys).

devongovett · 2018-07-02T17:39:50Z

sorry for the delay!

…differences in absolute dependency paths (parcel-bundler#1011)

… absolute dependency paths (#1011)

pcattori changed the title ~~fixes #933~~ fixes #933 : JSPackager deduplication now accounts for differences in absolute dependency paths Mar 17, 2018

devongovett requested changes Mar 18, 2018

View reviewed changes

devongovett requested changes Mar 28, 2018

View reviewed changes

henri-hulski mentioned this pull request Apr 11, 2018

Build without HMR fails with 'is not a function' type error #1152

Closed

henri-hulski mentioned this pull request Apr 16, 2018

Add test to not dedupe imports with same contents but different dependency paths #1198

Closed

olizilla mentioned this pull request May 8, 2018

Parcel fails to build js-ipfs (and many other things) ipfs/js-ipfs#1344

Closed

henri-hulski approved these changes May 16, 2018

View reviewed changes

DeMoorJasper added the 🐛 Bug label May 19, 2018

fathyb approved these changes May 24, 2018

View reviewed changes

DeMoorJasper approved these changes Jun 6, 2018

View reviewed changes

devongovett requested changes Jul 2, 2018

View reviewed changes

pcattori and others added 4 commits July 2, 2018 07:08

tests for JSPackager asset dedupe with hmr enabled (simulating `parce…

89d5d80

…l build`)

fixes #933

59e3d7a

asset dedupe tests construct paths via path.join for cross-platform…

a8060cf

… support

Use objectHash, pre-resolved paths, fix tests

c5356c6

devongovett approved these changes Jul 2, 2018

View reviewed changes

devongovett merged commit f699e81 into parcel-bundler:master Jul 2, 2018

pcattori deleted the dedup-aware-of-absolute-dependency-paths branch July 2, 2018 17:52

2zH pushed a commit to 2zH/parcel that referenced this pull request Jul 11, 2018

fixes parcel-bundler#933 : JSPackager deduplication now accounts for …

9c74786

…differences in absolute dependency paths (parcel-bundler#1011)

devongovett pushed a commit that referenced this pull request Oct 15, 2018

fixes #933 : JSPackager deduplication now accounts for differences in…

85194bc

… absolute dependency paths (#1011)

devongovett pushed a commit that referenced this pull request Oct 15, 2018

fixes #933 : JSPackager deduplication now accounts for differences in…

4f60d91

… absolute dependency paths (#1011)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #933 : JSPackager deduplication now accounts for differences in absolute dependency paths #1011

fixes #933 : JSPackager deduplication now accounts for differences in absolute dependency paths #1011

pcattori commented Mar 17, 2018 •

edited by DeMoorJasper

Loading

codecov-io commented Mar 17, 2018 •

edited

Loading

devongovett Mar 18, 2018

pcattori commented Mar 19, 2018

devongovett left a comment

devongovett Mar 28, 2018

henri-hulski Apr 15, 2018 •

edited

Loading

felixrabe May 6, 2018

henri-hulski May 7, 2018

felixrabe May 8, 2018

pcattori commented Mar 29, 2018 •

edited

Loading

henri-hulski commented Apr 11, 2018

henri-hulski commented Apr 16, 2018

henri-hulski commented Apr 18, 2018

pcattori commented Apr 18, 2018 •

edited

Loading

DeMoorJasper commented Apr 18, 2018

henri-hulski commented Apr 20, 2018

olizilla commented May 8, 2018

pcattori commented May 8, 2018

olizilla commented May 9, 2018 •

edited

Loading

pcattori commented May 13, 2018 •

edited

Loading

henri-hulski commented May 15, 2018

henri-hulski commented May 15, 2018

pcattori commented May 16, 2018

henri-hulski left a comment

olizilla commented May 31, 2018

pcattori commented Jun 6, 2018

asyncx commented Jun 12, 2018

henri-hulski commented Jun 20, 2018

olizilla commented Jun 26, 2018

pcattori commented Jun 26, 2018 •

edited

Loading

gaspard commented Jul 2, 2018

devongovett Jul 2, 2018

devongovett commented Jul 2, 2018

devongovett commented Jul 2, 2018

fixes #933 : JSPackager deduplication now accounts for differences in absolute dependency paths #1011

fixes #933 : JSPackager deduplication now accounts for differences in absolute dependency paths #1011

Conversation

pcattori commented Mar 17, 2018 • edited by DeMoorJasper Loading

codecov-io commented Mar 17, 2018 • edited Loading

Codecov Report

devongovett Mar 18, 2018

Choose a reason for hiding this comment

pcattori commented Mar 19, 2018

devongovett left a comment

Choose a reason for hiding this comment

devongovett Mar 28, 2018

Choose a reason for hiding this comment

henri-hulski Apr 15, 2018 • edited Loading

Choose a reason for hiding this comment

felixrabe May 6, 2018

Choose a reason for hiding this comment

henri-hulski May 7, 2018

Choose a reason for hiding this comment

felixrabe May 8, 2018

Choose a reason for hiding this comment

pcattori commented Mar 29, 2018 • edited Loading

henri-hulski commented Apr 11, 2018

henri-hulski commented Apr 16, 2018

henri-hulski commented Apr 18, 2018

pcattori commented Apr 18, 2018 • edited Loading

DeMoorJasper commented Apr 18, 2018

henri-hulski commented Apr 20, 2018

olizilla commented May 8, 2018

pcattori commented May 8, 2018

olizilla commented May 9, 2018 • edited Loading

pcattori commented May 13, 2018 • edited Loading

henri-hulski commented May 15, 2018

henri-hulski commented May 15, 2018

pcattori commented May 16, 2018

henri-hulski left a comment

Choose a reason for hiding this comment

olizilla commented May 31, 2018

pcattori commented Jun 6, 2018

asyncx commented Jun 12, 2018

henri-hulski commented Jun 20, 2018

olizilla commented Jun 26, 2018

pcattori commented Jun 26, 2018 • edited Loading

gaspard commented Jul 2, 2018

devongovett Jul 2, 2018

Choose a reason for hiding this comment

devongovett commented Jul 2, 2018

devongovett commented Jul 2, 2018

pcattori commented Mar 17, 2018 •

edited by DeMoorJasper

Loading

codecov-io commented Mar 17, 2018 •

edited

Loading

henri-hulski Apr 15, 2018 •

edited

Loading

pcattori commented Mar 29, 2018 •

edited

Loading

pcattori commented Apr 18, 2018 •

edited

Loading

olizilla commented May 9, 2018 •

edited

Loading

pcattori commented May 13, 2018 •

edited

Loading

pcattori commented Jun 26, 2018 •

edited

Loading