Fix dependencies bundling for yarn workspaces #1564

honzajavorek · 2019-10-18T15:33:25Z

🚀 Why this change?

This improves the hack which gets rid of protagonist (i.e. C++ compilation of drafter, the API Blueprint parser). When we moved dredd-transactions to the monorepo, where dependencies are managed by yarn workspaces, the hack to get rid of protagonist stopped to work. The hack relies on 'bundledDependencies', but those stopped to work. 'npm pack' cannot find the dependencies to bundle if they're in the root 'node_modules' directory. 'yarn pack' doesn't work either, it has bugs and it doesn't support 'bundledDependencies' or the workspaces very well - see yarnpkg/yarn#6794

This change overcomes the limitation by improving the prepack.js script. It now iterates over the 'bundledDependencies' and symlinks them to the local 'node_modules' directory where 'npm pack' can pick them up. After packing is done, there's a new postpack.js script, which makes sure the symlinks get cleaned up. The resulting .tgz then contains the dependencies bundled - without protagonist.

📝 Related issues and Pull Requests

#1551

✅ What didn't I forget?

To write docs
To write tests
To put Conventional Changelog prefixes in front of all my commits and run npm run lint

This improves the hack which gets rid of protagonist (i.e. C++ compilation of drafter, the API Blueprint parser). When we moved dredd-transactions to the monorepo, where dependencies are managed by yarn workspaces, the hack to get rid of protagonist stopped to work. The hack relies on 'bundledDependencies', but those stopped to work. 'npm pack' cannot find the dependencies to bundle if they're in the root 'node_modules' directory. 'yarn pack' doesn't work either, it has bugs and it doesn't support 'bundledDependencies' or the workspaces very well - see yarnpkg/yarn#6794 This change overcomes the limitation by improving the prepack.js script. It now iterates over the 'bundledDependencies' and symlinks them to the local 'node_modules' directory where 'npm pack' can pick them up. After packing is done, there's a new postpack.js script, which makes sure the symlinks get cleaned up. The resulting .tgz then contains the dependencies bundled - without protagonist.

…cy for some reason

artem-zakharchenko

The script itself is fine, but I feel terribly sorry we have to invent such a process to exclude a single dependency.

I will try to research this on Yarn side (it's pretty smart, and allows a few additional things when working with dependencies than npm does), but otherwise I would expect this to be fixed in drafter. Such dependencies distribution is strange.

packages/dredd-transactions/scripts/postpack.js

artem-zakharchenko · 2019-10-21T09:28:40Z

packages/dredd-transactions/scripts/prepack.js

+
+
+// make sure all bundled deps are accessible in the local 'node_modules' dir
+const packageData = readPackageJson(PACKAGE_DIR, 'package.json');


readPackageJson function requires 1 argument, given 2. I think you can remove 'package.json'.

artem-zakharchenko · 2019-10-21T09:50:32Z

Theoretically, it's possible to use Yarn custom resolutions to hijack which version of protagonist should be installed. It can work with 4th party dependencies too:

{
  "resolutions": {
    "fury-adapter-xxx/drafter/protagonist": "https://registry.npmjs.org/package-that-cannot-be-installed/-/package-that-cannot-be-installed-0.0.0.tgz"
  }
}

The question is whether Yarn supports such nested path (it's a 5th party dependency in the end). If it does, replacing the tarball that should get installed by a non-existing package should hack the packing and omit protagonist altogether. This will also affect the local installations of the package.

Edit: tried this, doesn't seem to be working.

artem-zakharchenko

Overlooked a few unused things. Please adjust and let's merge :) Thanks.

Edit: Committed changes in 096a322.

pksunkara

Looks good, just a few comments.

packages/dredd-transactions/package.json

pksunkara · 2019-10-21T11:40:27Z

packages/dredd-transactions/scripts/prepack.js

+function symlinkDependencyTreeToLocalNodeModules(dependencyName) {
+  const localDependencyPath = path.join(PACKAGE_DIR, 'node_modules', dependencyName);
+  if (!fs.existsSync(localDependencyPath)) {
+    fs.symlinkSync(`../../../node_modules/${dependencyName}`, localDependencyPath);


Maybe it's better to get the root of the project here, instead of doing ../../.. in case we change the directories later.

Since dredd-transactions package is always the direct child of the dredd/packages, the only thing that is variable is the location of the prepack.js script, which casts module requires.

I don't think it's worthy to invest time into this, as scripts are purely technical, and touching them should raise expectations that things may break.

Actually, the first argument of fs.symlinkSync() must be a relative path from the resulting symlink's location:

https://nodejs.org/api/fs.html#fs_fs_symlink_target_path_type_callback

I wouldn't be too concerned about this as if things change, the smoke test is our insurance.

pksunkara · 2019-10-21T11:41:26Z

packages/dredd-transactions/scripts/prepack.js

+// get rid of protagonist everywhere
+[
+  path.join(PACKAGE_DIR, 'node_modules', 'protagonist'),
+  path.join(PACKAGE_DIR, '..', '..', 'node_modules', 'protagonist'),


Same here about root project dir.

honzajavorek · 2019-10-21T15:15:22Z

Thanks for the additional fixes. I'm merging this.

honzajavorek added 2 commits October 18, 2019 17:38

chore: eslint doesn't allow prepack.js dependency to be a devDependen…

23c43e6

…cy for some reason

honzajavorek force-pushed the honzajavorek/fix-dependencies-bundling branch from d4f1532 to 23c43e6 Compare October 18, 2019 15:42

honzajavorek marked this pull request as ready for review October 18, 2019 15:50

honzajavorek requested a review from artem-zakharchenko October 18, 2019 15:50

artem-zakharchenko approved these changes Oct 21, 2019

View reviewed changes

artem-zakharchenko suggested changes Oct 21, 2019

View reviewed changes

fix: adjusts "readPackageJson" function call

096a322

artem-zakharchenko requested a review from pksunkara October 21, 2019 11:40

pksunkara approved these changes Oct 21, 2019

View reviewed changes

chore: moves "rimraf" to dev dependencies

2e203e0

artem-zakharchenko force-pushed the honzajavorek/fix-dependencies-bundling branch from 3acd07e to 2e203e0 Compare October 21, 2019 12:07

honzajavorek merged commit 2b9bbc7 into monoprepo-dredd-transactions Oct 21, 2019

honzajavorek deleted the honzajavorek/fix-dependencies-bundling branch October 21, 2019 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix dependencies bundling for yarn workspaces #1564

Fix dependencies bundling for yarn workspaces #1564

honzajavorek commented Oct 18, 2019

artem-zakharchenko left a comment

artem-zakharchenko Oct 21, 2019

artem-zakharchenko commented Oct 21, 2019 •

edited

Loading

artem-zakharchenko left a comment •

edited

Loading

pksunkara left a comment

pksunkara Oct 21, 2019

artem-zakharchenko Oct 21, 2019

honzajavorek Oct 21, 2019

pksunkara Oct 21, 2019

honzajavorek commented Oct 21, 2019



		// make sure all bundled deps are accessible in the local 'node_modules' dir
		const packageData = readPackageJson(PACKAGE_DIR, 'package.json');

Fix dependencies bundling for yarn workspaces #1564

Fix dependencies bundling for yarn workspaces #1564

Conversation

honzajavorek commented Oct 18, 2019

🚀 Why this change?

📝 Related issues and Pull Requests

✅ What didn't I forget?

artem-zakharchenko left a comment

Choose a reason for hiding this comment

artem-zakharchenko Oct 21, 2019

Choose a reason for hiding this comment

artem-zakharchenko commented Oct 21, 2019 • edited Loading

artem-zakharchenko left a comment • edited Loading

Choose a reason for hiding this comment

pksunkara left a comment

Choose a reason for hiding this comment

pksunkara Oct 21, 2019

Choose a reason for hiding this comment

artem-zakharchenko Oct 21, 2019

Choose a reason for hiding this comment

honzajavorek Oct 21, 2019

Choose a reason for hiding this comment

pksunkara Oct 21, 2019

Choose a reason for hiding this comment

honzajavorek commented Oct 21, 2019

artem-zakharchenko commented Oct 21, 2019 •

edited

Loading

artem-zakharchenko left a comment •

edited

Loading