Allow bundleDependencies to be respected during `pack` #5966

thoov · 2018-06-10T17:29:55Z

Summary

Yarn pack does not include bundleDependencies during yarn pack. This was due to 3 issues:

The normalized property is bundleDependencies and not bundledDependencies. This was causing the destructuring to always be undefined even if bundledDependencies or bundleDependencies were set in package.json.
The way the filtering logic is constructed excludes node_modules from the 'search space'. This leads the filtering to attempt to find dependencies but cant because they were excluded.
The logic was only set up to find the dependency but not that dependency's dependency tree. This would lead to an incomplete pack of the needed dependencies.

Fixing problem 1: was a simple property name change which you can see here: 8081ccd

For fixing problems 2 & 3: We take the specified bundleDependencies and construct the list of their dependency graphs. Once we have the list we figure out all of the needed files for each one of those (sub)dependencies. You can see that change here: 54830e4

Related Issue: #2636

Test plan

I ran this in comparison to npm pack on a few local projects of various sizes and compared the outputs and performance (right now there is also an issue with bundleDependecies and npm so we will need to use npm 5.3.0 as that is the last working version).

The outputs look correct but I do notice a perf hit compared to npm pack. This perf slowness does not seem to be related to these changes directly but on the library that created the tar. On larger projects that I tested it would take npm pack about 15 seconds and for yarn pack it was 45 seconds.

buildsize · 2018-06-10T17:32:43Z

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	925.41 KB	930.47 KB	5.06 KB (1%)
yarn-[version].js	4 MB	4.02 MB	23.5 KB (1%)
yarn-legacy-[version].js	4.16 MB	4.19 MB	23.61 KB (1%)
yarn-v[version].tar.gz	930.5 KB	935.81 KB	5.31 KB (1%)
yarn_[version]all.deb	685.19 KB	689.12 KB	3.93 KB (1%)

arcanis · 2018-06-11T09:44:40Z

Pack-related tests seem to fail.

Btw, I'm a bit curious, what are you using bundleDependencies for?

stefanpenner · 2018-06-11T15:06:39Z

Btw, I'm a bit curious, what are you using bundleDependencies for?

A deployable tarball.

thoov · 2018-06-12T06:22:28Z

@arcanis Tests should now be passing (except for appveyor but they seem to be failing before my change). As @stefanpenner said bundleDependencies are needed for us to create deployable tarball that is completely self contained.

stefanpenner · 2018-06-12T23:02:15Z

@thoov mind comparing appveyer before/after and confirming its not worse then before? That should help the yarn maintainers a tad.

Walk the bundleDependency tree and include the related files.

thoov · 2018-06-13T03:12:12Z

@stefanpenner @arcanis Ok now tests are fully passing

thoov · 2018-06-27T16:56:41Z

@arcanis Could you provide a review or do you know who would be best to take a look at this PR?

byF · 2018-07-12T10:12:46Z

Can anyone review and approve this please?

arcanis · 2018-07-12T10:41:53Z

src/cli/commands/pack.js

+  let bundleDependenciesFiles = [];
+  if (bundleDependencies) {
+    for (const dependency of bundleDependencies) {
+      const dependencyList = depsFor(dependency, config.cwd);


What is this depsFor used for? Why can't we just walk the node_modules/${dependency} tree?

depsFor simply returns a list of all of the dependencies for a given package so in a sense it is "walking the tree". I agree with your concerns about not adding unnecessary extra packages to the project. Do you know if yarn has a built in util for getting this info? If not I think it is safe to bring this package in.

What I don't understand is: wouldn't something like this work without depsFor (pseudo code)?

const FOLDERS_IGNORE_NO_NODE_MODULES = new Set(FOLDERS_IGNORE); FOLDERS_IGNORE_NO_NODE_MODULES.delete('node_modules'); for (const dependency of bundleDependencies) { const depPath = getPathForDependency(dependency); const filesForBundledDep = await fs.walk(depPath, null, new Set(FOLDERS_IGNORE_NO_NODE_MODULES)); bundleDependenciesFiles = bundleDependenciesFiles.concat(filesForBundledDep); }

Since walk would then traverse the whole node_modules tree and pack them all.

@arcanis The issue is that this pseudo code doesn't actually walk the dependency graph for a dependency and only walks the directory structure. If we have a dependency a which has b and c as dependencies we get this tree structure:

node_modules/a
node_modules/b
node_modules/c

And const depPath = getPathForDependency(dependency); would only walk node_modules/a and miss the other ones. Thats why I use depsFor as I need more than the "top" level bundled dependencies. Let me know if this make sense or if there is something I am missing.

I see, that makes sense. I'm not fond of adding a new dep, but I don't see an easy replacement at the moment, so we can start with that.

arcanis · 2018-07-12T10:44:08Z

Hey, sorry for the delay, I'd like to know more about the depsFor function. It seems to come from a package without much usage, that itself uses dependencies without much usage. Unless there's a good reason to use it, I'd prefer to be careful about the dependencies we add to the project.

thoov · 2018-07-12T20:56:58Z

Thanks for the comments @arcanis. I left a response inline to your concern above.

arcanis · 2018-07-19T15:30:43Z

src/cli/commands/pack.js

+  let bundleDependenciesFiles = [];
+  if (bundleDependencies) {
+    for (const dependency of bundleDependencies) {
+      const dependencyList = depsFor(dependency, config.cwd);


What I don't understand is: wouldn't something like this work without depsFor (pseudo code)?

const FOLDERS_IGNORE_NO_NODE_MODULES = new Set(FOLDERS_IGNORE); FOLDERS_IGNORE_NO_NODE_MODULES.delete('node_modules'); for (const dependency of bundleDependencies) { const depPath = getPathForDependency(dependency); const filesForBundledDep = await fs.walk(depPath, null, new Set(FOLDERS_IGNORE_NO_NODE_MODULES)); bundleDependenciesFiles = bundleDependenciesFiles.concat(filesForBundledDep); }

Since walk would then traverse the whole node_modules tree and pack them all.

arcanis · 2018-07-25T08:09:59Z

Merged. Thanks!

When running `yarn pack` with a bundledDependency yarn was throwing an error: "error An unexpected error occurred: "Cannot find module \".\""." This was due to yarn's cli.js being transpiled to on line 110321: ``` var thePackage = !(function webpackMissingModule() { var e = new Error("Cannot find module \".\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()); ``` This line comes from [here](https://github.com/stefanpenner/hash-for-dep/blob/e849d3e2f0350810716a0e504c84603362229570/lib/pkg.js#L20) and is a dynamic require. This commit impements a fix for this based on the guidence from [this issue](webpack/webpack#4175 (comment)) This code was originally added with yarnpkg#5966

When running `yarn pack` with a bundledDependency yarn was throwing an error: "error An unexpected error occurred: "Cannot find module \".\""." This was due to yarn's cli.js being transpiled to on line 110321: ``` var thePackage = !(function webpackMissingModule() { var e = new Error("Cannot find module \".\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()); ``` This line comes from https://github.com/stefanpenner/hash-for-dep/blob/e849d3e2f0350810716a0e504c84603362229570/lib/pkg.js#L20 and is a dynamic require. This commit impements a fix for this based on the guidence from webpack/webpack#4175 (comment) This code was originally added with yarnpkg#5966

When running `yarn pack` with a bundledDependency yarn was throwing an error: An unexpected error occurred: "Cannot find module \".\"". This was due to yarn's cli.js being transpiled to on line 110321: ``` var thePackage = !(function webpackMissingModule() { var e = new Error("Cannot find module \".\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()); ``` This line comes from https://github.com/stefanpenner/hash-for-dep/blob/e849d3e2f0350810716a0e504c84603362229570/lib/pkg.js#L20 and is a dynamic require. This commit impements a fix for this based on the guidence from webpack/webpack#4175 (comment) This code was originally added with yarnpkg#5966

* Fixing dynamic require missing from webpack When running `yarn pack` with a bundledDependency yarn was throwing an error: An unexpected error occurred: "Cannot find module \".\"". This was due to yarn's cli.js being transpiled to on line 110321: ``` var thePackage = !(function webpackMissingModule() { var e = new Error("Cannot find module \".\""); e.code = 'MODULE_NOT_FOUND'; throw e; }()); ``` This line comes from https://github.com/stefanpenner/hash-for-dep/blob/e849d3e2f0350810716a0e504c84603362229570/lib/pkg.js#L20 and is a dynamic require. This commit impements a fix for this based on the guidence from webpack/webpack#4175 (comment) This code was originally added with #5966 * Update CHANGELOG.md

thoov added 5 commits June 12, 2018 19:26

Rename bundledDependencies to bundleDependencies

3174b5d

bundleDependencies respected during pack

6650642

Walk the bundleDependency tree and include the related files.

Unskip test for bundledDependencies

4cd9345

Fixing test case as os.tempdir would return a symlink

babda35

Make tests pass on windows

9479172

thoov force-pushed the yarn-pack-improvements branch from df2e6cb to 9479172 Compare June 13, 2018 02:35

arcanis requested changes Jul 12, 2018

View reviewed changes

arcanis requested changes Jul 19, 2018

View reviewed changes

arcanis approved these changes Jul 25, 2018

View reviewed changes

arcanis merged commit dda0217 into yarnpkg:master Jul 25, 2018

thoov mentioned this pull request Jan 11, 2019

Fixing dynamic require missing from webpack #6908

Merged

paul-soporan mentioned this pull request Jan 3, 2021

bundledDependencies don't get packed as of >= v0.19.0 #2636

Closed

Barsnes mentioned this pull request Nov 2, 2023

Fix use of yarn pack digdir/designsystemet#1019

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow bundleDependencies to be respected during `pack` #5966

Allow bundleDependencies to be respected during `pack` #5966

thoov commented Jun 10, 2018

buildsize bot commented Jun 10, 2018 •

edited

Loading

arcanis commented Jun 11, 2018

stefanpenner commented Jun 11, 2018

thoov commented Jun 12, 2018

stefanpenner commented Jun 12, 2018

thoov commented Jun 13, 2018

thoov commented Jun 27, 2018

byF commented Jul 12, 2018

arcanis Jul 12, 2018

thoov Jul 12, 2018

arcanis Jul 19, 2018 •

edited

Loading

thoov Jul 25, 2018

arcanis Jul 25, 2018

arcanis commented Jul 12, 2018

thoov commented Jul 12, 2018

arcanis Jul 19, 2018 •

edited

Loading

arcanis commented Jul 25, 2018

Allow bundleDependencies to be respected during pack #5966

Allow bundleDependencies to be respected during pack #5966

Conversation

thoov commented Jun 10, 2018

buildsize bot commented Jun 10, 2018 • edited Loading

arcanis commented Jun 11, 2018

stefanpenner commented Jun 11, 2018

thoov commented Jun 12, 2018

stefanpenner commented Jun 12, 2018

thoov commented Jun 13, 2018

thoov commented Jun 27, 2018

byF commented Jul 12, 2018

arcanis Jul 12, 2018

Choose a reason for hiding this comment

thoov Jul 12, 2018

Choose a reason for hiding this comment

arcanis Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

thoov Jul 25, 2018

Choose a reason for hiding this comment

arcanis Jul 25, 2018

Choose a reason for hiding this comment

arcanis commented Jul 12, 2018

thoov commented Jul 12, 2018

arcanis Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

arcanis commented Jul 25, 2018

Allow bundleDependencies to be respected during `pack` #5966

Allow bundleDependencies to be respected during `pack` #5966

buildsize bot commented Jun 10, 2018 •

edited

Loading

arcanis Jul 19, 2018 •

edited

Loading

arcanis Jul 19, 2018 •

edited

Loading