Adding missing dependencies to package.json files to clean up phantom dependencies #8604

alshdavid · 2022-11-03T22:21:45Z

Background

Yarn workspaces flattens all dependencies declared in the workspace packages into the top level node_modeules. This, combined with Node's module resolution strategy, allows packages in the workspace to have access to a dependency declared in another package's package.json without declaring that dependency in their own package.json.

These are known as phantom dependencies and can cause friction when it comes the versioning of dependencies, unexpected breakages for consumers due to missing packages (that are available in the workspace but not when pulled from npm), can sometimes hide circular dependencies and cause various other annoyances.

CHANGELOG

Identified the dependencies of each project and included those dependencies in its package.json.
Identified packages that explicitly need hoisting and included them in the .npmcrc
- We should probably correct the packages that require hoisting

Notes

I am not 100% sure what should be a devDependency, peerDependency and dependency as I lack the context on what is a library and what is a consumer. Please check my working in this regard

mischnic · 2022-11-03T22:29:32Z

We also have this eslint rule which I would have expected to catch these undeclared dependencies (I removed nullthrows from the package.json):

parcel/packages/core/diagnostic/src/diagnostic.js
  4:24  error  'nullthrows' should be listed in the project's dependencies. Run 'npm i -S nullthrows' to add it  import/no-extraneous-dependencies

alshdavid · 2022-11-03T22:30:45Z

packages/core/test-utils/package.json

    "@parcel/utils": "2.7.0",
    "chalk": "^4.1.0",
    "ncp": "^2.0.0",
    "nullthrows": "^1.1.1",
    "posthtml": "^0.16.5",
    "posthtml-parser": "^0.10.1",
-    "resolve": "^1.12.0",
+    "resolve": "^1.22.1",


Updated this to the lasted minor version as there were issues with linux-arm64

alshdavid · 2022-11-03T22:31:50Z

packages/dev/eslint-config-browser/package.json

@@ -7,6 +7,6 @@
  },
  "peerDependencies": {
    "@babel/core": "^7.0.0",
-    "eslint": ">= 7.0.0"
+    "eslint": "^7.32.0"


Had to ensure eslint was the same version in all packages that use it due to it being a global workspace package.

alshdavid · 2022-11-03T22:33:26Z

packages/optimizers/image/package.json

@@ -39,6 +39,7 @@
    "detect-libc": "^1.0.3"
  },
  "devDependencies": {
+    "@types/node": "^12.12.6",


This should probably be added to packages that specifically need node types rather than relying on the global package on the top level

alshdavid · 2022-11-03T22:34:43Z

packages/transformers/elm/package.json

@@ -29,6 +29,9 @@
    "nullthrows": "^1.1.1",
    "terser": "^5.14.2"
  },
+  "devDependencies": {
+    "@parcel/types": "2.7.0"
+  },
  "peerDependencies": {
    "elm": "^0.19.1-5"


Worth noting that elm does not work on linux-arm64 meaning Parcel cannot be built/developed on docker for MacOS, ARM Linux, Asahi, etc

elm/compiler#2234

EDIT: Notified the elm team, they are addressing this

alshdavid · 2022-11-03T22:42:19Z

packages/utils/parcel-lsp/package.json

-    "@parcel/diagnostic": "2.5.0",
-    "@parcel/plugin": "2.5.0",
-    "@parcel/types": "2.5.0",
-    "@parcel/utils": "2.5.0",


Were these intentionally set to use old versions of parcel?

If we are interested in syncing all packages in the repo against their declared workspace version, pnpm has phenomenal workspace support that avoids phantom dependencies and allows us to describe package dependencies with:

"dependencies": { "@parcel/types": "workspace/*" }

which automatically transforms the version to the version declared in the workspace package on package publishing.

"dependencies": { "@parcel/types": "2.7.0" }

If a dependency with a different version is required, it installs it locally to the package without creating conflicts with other packages

Were these intentionally set to use old versions of parcel?

No, but for some reason this step didn't bump the version on the last release

parcel/package.json

Line 33 in 932b6bd

"tag:release": "lerna version --exact --force-publish=* --no-git-tag-version --no-push && yarn adjust-versions",

"@parcel/types": "workspace/*"

Is there some mechanism to replace that with the actual version on publish? Or is that only meant for private packages?

Is there some mechanism to replace that with the actual version on publish? Or is that only meant for private packages?

It's a mechanism to replace the version with the actual version when running pnpm pack or pnpm publish - it works for public or private packages as it's just string replacement when publishing.

Reduces the maintenance burden involved in tracking the versions of packages in the workspace and avoids us needing to commit absolute versions in the source repo which has caused me so much grief in the past 🤣

To be clear, this is a feature of pnpm workspaces and I don't believe it's available in lerna or yarn - so it's currently unavailable to us, but is that something we'd be open to discussing?

parcel-benchmark · 2022-11-03T23:03:17Z

Benchmark Results

Kitchen Sink ✅

Timings

Description	Time	Difference
Cold	1.42s	-7.00ms
Cached	350.00ms	+5.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

React HackerNews ✅

Timings

Description	Time	Difference
Cold	9.07s	-36.00ms
Cached	431.00ms	+7.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

AtlasKit Editor ✅

Timings

Description	Time	Difference
Cold	1.59m	-1.17s
Cached	2.13s	-38.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Three.js ✅

Timings

Description	Time	Difference
Cold	6.50s	-79.00ms
Cached	255.00ms	+5.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

package.json

alshdavid · 2022-11-03T23:32:57Z

package.json

    "flow-bin": "0.184.0",
    "glob": "^7.1.6",
    "gulp": "^4.0.2",
    "gulp-babel": "^8.0.0",
    "husky": "^6.0.0",
    "lerna": "^3.22.1",
+    "@octokit/core": "3.6.0",


This is a peerDependency of lerna, adding this to suppress a warning

alshdavid · 2022-11-04T00:09:13Z

packages/runtimes/webextension/src/autoreload-bg.js

@@ -1,4 +1,4 @@
-/* global chrome, browser, addEventListener, fetch, Response, HMR_HOST, HMR_PORT */
+/* global chrome, browser, addEventListener, Response, HMR_HOST, HMR_PORT */


Linting error

1:46 error 'fetch' is already defined as a built-in global variable no-redeclare

alshdavid · 2022-11-04T00:09:34Z

packages/core/utils/src/shared-buffer.js

@@ -1,5 +1,4 @@
 // @flow
-/* global MessageChannel:readonly */


Linting error

1:46 error 'MessageChannel' is already defined as a built-in global variable no-redeclare

alshdavid · 2022-11-04T00:16:49Z

package.json

    "@khanacademy/flow-to-ts": "^0.5.2",
    "@napi-rs/cli": "^2.6.2",
-    "@parcel/babel-register": "2.7.0",
-    "@types/node": "^15.12.4",
+    "@types/node": "^12.20.55",


Making sure our node types are aligned with our engine support

package.json

alshdavid · 2022-11-04T00:24:14Z

@mischnic: We also have this eslint rule which I would have expected to catch these undeclared dependencies (I removed nullthrows from the package.json)

In a previous project I worked on we also used an eslint rule to enforce packages explicitly declaring their dependencies and it worked pretty well. Also helped with blacklisting imports containing /src/ and such.

We ended up moving to pnpm as, in workspaces, it structures node_modules in a way that ensures it's impossible to use a dependency without declaring it (each package has its own node_modules with symlinks to the actual packages - results in 0kb node_modules folders, instant&offline installs and super safe workspaces)

alshdavid · 2022-11-04T02:03:25Z

Should I do a patch version bump on the packages that have been updated?

mischnic · 2022-11-04T10:47:53Z

Should I do a patch version bump on the packages that have been updated?

No, we do the version bumping as part of the release process.

In a previous project I worked on we also used an eslint rule to enforce packages explicitly declaring their dependencies and it worked pretty well. Also helped with blacklisting imports containing /src/ and such.

It'd also say that it worked pretty well for Parcel so far. But apparently it's not working for intra-workspace packges (which the current eslint rule apparently silently allows)

We ended up moving to pnpm

I guess that's a separate discussion. Detecting these undeclared dependencies statically in a linter seems better to me than relying on the import call to fail at runtime/in the tests.

alshdavid · 2022-11-07T06:21:17Z

packages/optimizers/image/src/ImageOptimizer.js

@@ -1,6 +1,5 @@
 // @flow
 import path from 'path';
-import process from 'process';


Flow error Cannot resolve module 'process'.

alshdavid · 2022-11-07T06:29:02Z

package.json

@@ -37,6 +37,9 @@
  },
  "devDependencies": {
    "@babel/core": "^7.12.0",
+    "@octokit/core": "3.6.0",
+    "@parcel/eslint-config": "2.7.0",
+    "@parcel/babel-preset": "2.7.0",


@octokit/core is an unmet peer dependency of lerna it was complaining about
@parcel/eslint-config Is used by ./.eslintrc.json
@parcel/babel-preset Is used by ./babel.config.json

alshdavid · 2022-11-07T06:40:40Z

packages/dev/babel-preset/index.js

@@ -55,7 +56,7 @@ module.exports = api => {
              ],
            },
          ],
-          'babel-plugin-minify-dead-code-elimination',
+          require('babel-plugin-minify-dead-code-elimination'),


These were causing resolution issues when strict/isolated workspace packages were enabled. They now resolve relative to this package and will always use the dependencies of this package first

alshdavid requested review from devongovett, thebriando, mischnic and a team and removed request for devongovett, thebriando and mischnic November 3, 2022 22:21

alshdavid force-pushed the alsh/rationalizing-phantom-dependencies branch from 3caa91d to 9e0d86e Compare November 3, 2022 22:22

alshdavid commented Nov 3, 2022

View reviewed changes

alshdavid force-pushed the alsh/rationalizing-phantom-dependencies branch from 33d20da to fa92482 Compare November 3, 2022 23:29

alshdavid commented Nov 3, 2022

View reviewed changes

package.json Outdated Show resolved Hide resolved

alshdavid commented Nov 3, 2022

View reviewed changes

alshdavid force-pushed the alsh/rationalizing-phantom-dependencies branch 4 times, most recently from ea18091 to 8232c11 Compare November 4, 2022 00:08

alshdavid commented Nov 4, 2022

View reviewed changes

alshdavid force-pushed the alsh/rationalizing-phantom-dependencies branch from 8232c11 to 9bf8258 Compare November 4, 2022 00:16

alshdavid commented Nov 4, 2022

View reviewed changes

package.json Show resolved Hide resolved

alshdavid force-pushed the alsh/rationalizing-phantom-dependencies branch 2 times, most recently from 6cfa714 to f707f35 Compare November 4, 2022 01:58

alshdavid force-pushed the alsh/rationalizing-phantom-dependencies branch 4 times, most recently from 2657c5a to e1be976 Compare November 7, 2022 06:19

alshdavid commented Nov 7, 2022

View reviewed changes

alshdavid force-pushed the alsh/rationalizing-phantom-dependencies branch 3 times, most recently from 1162feb to bd0b1af Compare November 7, 2022 06:28

alshdavid commented Nov 7, 2022

View reviewed changes

alshdavid force-pushed the alsh/rationalizing-phantom-dependencies branch from bd0b1af to 2ce79f4 Compare November 7, 2022 06:34

alshdavid commented Nov 7, 2022

View reviewed changes

alshdavid force-pushed the alsh/rationalizing-phantom-dependencies branch 9 times, most recently from 119a128 to 04c5090 Compare November 14, 2022 00:52

Updated packages to include their imports

669cc16

alshdavid closed this Nov 14, 2022

alshdavid force-pushed the alsh/rationalizing-phantom-dependencies branch from 04c5090 to 669cc16 Compare November 14, 2022 00:56

alshdavid deleted the alsh/rationalizing-phantom-dependencies branch November 14, 2022 00:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding missing dependencies to package.json files to clean up phantom dependencies #8604

Adding missing dependencies to package.json files to clean up phantom dependencies #8604

alshdavid commented Nov 3, 2022 •

edited

Loading

mischnic commented Nov 3, 2022

alshdavid Nov 3, 2022

alshdavid Nov 3, 2022

alshdavid Nov 3, 2022 •

edited

Loading

alshdavid Nov 3, 2022 •

edited

Loading

alshdavid Nov 3, 2022

mischnic Nov 4, 2022

alshdavid Nov 5, 2022

parcel-benchmark commented Nov 3, 2022 •

edited

Loading

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

alshdavid Nov 3, 2022

alshdavid Nov 4, 2022

alshdavid Nov 4, 2022

alshdavid Nov 4, 2022

alshdavid commented Nov 4, 2022 •

edited

Loading

alshdavid commented Nov 4, 2022

mischnic commented Nov 4, 2022

alshdavid Nov 7, 2022

alshdavid Nov 7, 2022 •

edited

Loading

alshdavid Nov 7, 2022

		@@ -1,4 +1,4 @@
		/* global chrome, browser, addEventListener, fetch, Response, HMR_HOST, HMR_PORT */
		/* global chrome, browser, addEventListener, Response, HMR_HOST, HMR_PORT */

Adding missing dependencies to package.json files to clean up phantom dependencies #8604

Adding missing dependencies to package.json files to clean up phantom dependencies #8604

Conversation

alshdavid commented Nov 3, 2022 • edited Loading

Background

CHANGELOG

Notes

mischnic commented Nov 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshdavid Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

alshdavid Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parcel-benchmark commented Nov 3, 2022 • edited Loading

Benchmark Results

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshdavid commented Nov 4, 2022 • edited Loading

alshdavid commented Nov 4, 2022

mischnic commented Nov 4, 2022

Choose a reason for hiding this comment

alshdavid Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshdavid commented Nov 3, 2022 •

edited

Loading

alshdavid Nov 3, 2022 •

edited

Loading

alshdavid Nov 3, 2022 •

edited

Loading

parcel-benchmark commented Nov 3, 2022 •

edited

Loading

alshdavid commented Nov 4, 2022 •

edited

Loading

alshdavid Nov 7, 2022 •

edited

Loading