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.

Sign up for GitHub

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

Allow bundleDependencies to be respected during pack #5966

Merged
merged 5 commits into from
Jul 25, 2018
Merged

Allow bundleDependencies to be respected during pack #5966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3174b5d
Rename bundledDependencies to bundleDependencies
thoov Jun 6, 2018
6650642
bundleDependencies respected during pack
thoov Jun 7, 2018
4cd9345
Unskip test for bundledDependencies
thoov Jun 10, 2018
babda35
Fixing test case as os.tempdir would return a symlink
thoov Jun 11, 2018
9479172
Make tests pass on windows
thoov Jun 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions __tests__/commands/pack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,7 @@ test.concurrent('pack should exclude all files in dot-directories if not in file
});
});

// Broken: https://github.com/yarnpkg/yarn/issues/2636
test.skip('pack should include bundled dependencies', (): Promise<void> => {
test.concurrent('pack should include bundled dependencies', (): Promise<void> => {
return runPack([], {}, 'bundled-dependencies', async (config): Promise<void> => {
const {cwd} = config;
const files = await getFilesFromArchive(
Expand All @@ -171,6 +170,9 @@ test.skip('pack should include bundled dependencies', (): Promise<void> => {
const expected = [
'index.js',
'package.json',
'node_modules',
path.join('node_modules', 'a'),
path.join('node_modules', 'b'),
path.join('node_modules', 'a', 'package.json'),
path.join('node_modules', 'b', 'package.json'),
];
Expand Down
7 changes: 7 additions & 0 deletions __tests__/fixtures/pack/bundled-dependencies/node_modules/a/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions __tests__/fixtures/pack/bundled-dependencies/node_modules/b/package.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"dnscache": "^1.0.1",
"glob": "^7.1.1",
"gunzip-maybe": "^1.4.0",
"hash-for-dep": "^1.2.3",
"ini": "^1.3.4",
"inquirer": "^3.0.1",
"invariant": "^2.2.0",
Expand Down
24 changes: 19 additions & 5 deletions src/cli/commands/pack.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const zlib = require('zlib');
const path = require('path');
const tar = require('tar-fs');
const fs2 = require('fs');
const depsFor = require('hash-for-dep/lib/deps-for');

const FOLDERS_IGNORE = [
// never allow version control folders
Expand Down Expand Up @@ -53,7 +54,7 @@ export async function packTarball(
{mapHeader}: {mapHeader?: Object => Object} = {},
): Promise<stream$Duplex> {
const pkg = await config.readRootManifest();
const {bundledDependencies, main, files: onlyFiles} = pkg;
const {bundleDependencies, main, files: onlyFiles} = pkg;

// include required files
let filters: Array<IgnoreFilter> = NEVER_IGNORE.slice();
Expand All @@ -65,10 +66,17 @@ export async function packTarball(
filters = filters.concat(ignoreLinesToRegex(['!/' + main]));
}

// include bundledDependencies
if (bundledDependencies) {
const folder = config.getFolder(pkg);
filters = ignoreLinesToRegex(bundledDependencies.map((name): string => `!${folder}/${name}`), '.');
// include bundleDependencies
let bundleDependenciesFiles = [];
if (bundleDependencies) {
for (const dependency of bundleDependencies) {
const dependencyList = depsFor(dependency, config.cwd);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@arcanis arcanis Jul 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


for (const dep of dependencyList) {
const filesForBundledDep = await fs.walk(dep.baseDir, null, new Set(FOLDERS_IGNORE));
bundleDependenciesFiles = bundleDependenciesFiles.concat(filesForBundledDep);
}
}
}

// `files` field
Expand Down Expand Up @@ -109,6 +117,12 @@ export async function packTarball(
// apply filters
sortFilter(files, filters, keepFiles, possibleKeepFiles, ignoredFiles);

// add the files for the bundled dependencies to the set of files to keep
for (const file of bundleDependenciesFiles) {
const realPath = await fs.realpath(config.cwd);
keepFiles.add(path.relative(realPath, file.absolute));
}

return packWithIgnoreAndHeaders(
config.cwd,
name => {
Expand Down
61 changes: 55 additions & 6 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1254,6 +1254,13 @@ braces@^2.3.0, braces@^2.3.1:
split-string "^3.0.2"
to-regex "^3.0.1"

broccoli-kitchen-sink-helpers@^0.3.1:
version "0.3.1"
resolved "https://registry.yarnpkg.com/broccoli-kitchen-sink-helpers/-/broccoli-kitchen-sink-helpers-0.3.1.tgz#77c7c18194b9664163ec4fcee2793444926e0c06"
dependencies:
glob "^5.0.10"
mkdirp "^0.5.1"

brorand@^1.0.1:
version "1.1.0"
resolved "https://registry.yarnpkg.com/brorand/-/brorand-1.1.0.tgz#12c25efe40a45e3c323eb8675a0a0ce57b22371f"
Expand Down Expand Up @@ -2883,6 +2890,16 @@ glob@^4.3.1:
minimatch "^2.0.1"
once "^1.3.0"

glob@^5.0.10:
version "5.0.15"
resolved "https://registry.yarnpkg.com/glob/-/glob-5.0.15.tgz#1bc936b9e02f4a603fcc222ecf7633d30b8b93b1"
dependencies:
inflight "^1.0.4"
inherits "2"
minimatch "2 || 3"
once "^1.3.0"
path-is-absolute "^1.0.0"

glob@^7.0.0, glob@^7.0.3, glob@^7.0.5, glob@^7.1.1, glob@^7.1.2:
version "7.1.2"
resolved "https://registry.yarnpkg.com/glob/-/glob-7.1.2.tgz#c19c9df9a028702d678612384a6552404c636d15"
Expand Down Expand Up @@ -3202,6 +3219,15 @@ hash-base@^2.0.0:
dependencies:
inherits "^2.0.1"

hash-for-dep@^1.2.3:
version "1.2.3"
resolved "https://registry.yarnpkg.com/hash-for-dep/-/hash-for-dep-1.2.3.tgz#5ec69fca32c23523972d52acb5bb65ffc3664cab"
dependencies:
broccoli-kitchen-sink-helpers "^0.3.1"
heimdalljs "^0.2.3"
heimdalljs-logger "^0.1.7"
resolve "^1.4.0"

hash.js@^1.0.0, hash.js@^1.0.3:
version "1.0.3"
resolved "https://registry.yarnpkg.com/hash.js/-/hash.js-1.0.3.tgz#1332ff00156c0a0ffdd8236013d07b77a0451573"
Expand All @@ -3226,6 +3252,19 @@ hawk@~6.0.2:
hoek "4.x.x"
sntp "2.x.x"

heimdalljs-logger@^0.1.7:
version "0.1.9"
resolved "https://registry.yarnpkg.com/heimdalljs-logger/-/heimdalljs-logger-0.1.9.tgz#d76ada4e45b7bb6f786fc9c010a68eb2e2faf176"
dependencies:
debug "^2.2.0"
heimdalljs "^0.2.0"

heimdalljs@^0.2.0, heimdalljs@^0.2.3:
version "0.2.5"
resolved "https://registry.yarnpkg.com/heimdalljs/-/heimdalljs-0.2.5.tgz#6aa54308eee793b642cff9cf94781445f37730ac"
dependencies:
rsvp "~3.2.1"

hmac-drbg@^1.0.0:
version "1.0.1"
resolved "https://registry.yarnpkg.com/hmac-drbg/-/hmac-drbg-1.0.1.tgz#d2745701025a6c775a6c545793ed502fc0c649a1"
Expand Down Expand Up @@ -4559,18 +4598,18 @@ minimalistic-crypto-utils@^1.0.0, minimalistic-crypto-utils@^1.0.1:
version "1.0.1"
resolved "https://registry.yarnpkg.com/minimalistic-crypto-utils/-/minimalistic-crypto-utils-1.0.1.tgz#f6c00c1c0b082246e5c4d99dfb8c7c083b2b582a"

"minimatch@2 || 3", minimatch@^3.0.0, minimatch@^3.0.2, minimatch@^3.0.3, minimatch@^3.0.4:
version "3.0.4"
resolved "https://registry.yarnpkg.com/minimatch/-/minimatch-3.0.4.tgz#5166e286457f03306064be5497e8dbb0c3d32083"
dependencies:
brace-expansion "^1.1.7"

minimatch@^2.0.1:
version "2.0.10"
resolved "https://registry.yarnpkg.com/minimatch/-/minimatch-2.0.10.tgz#8d087c39c6b38c001b97fca7ce6d0e1e80afbac7"
dependencies:
brace-expansion "^1.0.0"

minimatch@^3.0.0, minimatch@^3.0.2, minimatch@^3.0.3, minimatch@^3.0.4:
version "3.0.4"
resolved "https://registry.yarnpkg.com/minimatch/-/minimatch-3.0.4.tgz#5166e286457f03306064be5497e8dbb0c3d32083"
dependencies:
brace-expansion "^1.1.7"

minimatch@~0.2.11:
version "0.2.14"
resolved "https://registry.yarnpkg.com/minimatch/-/minimatch-0.2.14.tgz#c74e780574f63c6f9a090e90efbe6ef53a6a756a"
Expand Down Expand Up @@ -5552,6 +5591,12 @@ resolve@^1.1.6, resolve@^1.1.7:
dependencies:
path-parse "^1.0.5"

resolve@^1.4.0:
version "1.7.1"
resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.7.1.tgz#aadd656374fd298aee895bc026b8297418677fd3"
dependencies:
path-parse "^1.0.5"

restore-cursor@^1.0.1:
version "1.0.1"
resolved "https://registry.yarnpkg.com/restore-cursor/-/restore-cursor-1.0.1.tgz#34661f46886327fed2991479152252df92daa541"
Expand Down Expand Up @@ -5607,6 +5652,10 @@ ripemd160@^2.0.0, ripemd160@^2.0.1:
hash-base "^2.0.0"
inherits "^2.0.1"

rsvp@~3.2.1:
version "3.2.1"
resolved "https://registry.yarnpkg.com/rsvp/-/rsvp-3.2.1.tgz#07cb4a5df25add9e826ebc67dcc9fd89db27d84a"

run-async@^2.2.0:
version "2.3.0"
resolved "https://registry.yarnpkg.com/run-async/-/run-async-2.3.0.tgz#0371ab4ae0bdd720d4166d7dfda64ff7a445a6c0"
Expand Down