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.

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

Verify version when resolving Node builtin polyfills #8387

Merged
merged 9 commits into from Sep 3, 2022

Conversation

mischnic
Copy link
Member

@mischnic mischnic commented Aug 10, 2022

This will call packageManager.resolve for every resolution of a Node builtin with the correct range that should be used to catch cases where another instance of some Node builtin polyfill was pulled in by some other dependency, be it in the same package, or in a monorepo situation

If it's the wrong version, it will either autoinstall, or error (using the existing autoinstall logic):

@parcel/core: Failed to resolve 'util' from './a/index.js'
  /Users/nmischkulnig/Desktop/monorepo-builtin-versioning/a/index.js:1:20
  > 1 | import * as x from "util";
  >   |                    ^^^^^^
    2 |
    3 | console.log(x);


@parcel/package-manager: Could not find module "util" satisfying ^0.12.3.

  /Users/nmischkulnig/Desktop/monorepo-builtin-versioning/a/package.json:6:3
    5 |   "dependencies": {
  > 6 |     "util": "^0.4.9"
  >   |     ^^^^^^ Found this conflicting local requirement.
    7 |   }
    8 | }

Questions:

  • Previously, the polyfill was resolved from the actual source file but autoinstalled into the project root. We either need to install it into the local package (what I've done now), or resolve it relative from the monorepo root. (So that resolution base = autoinstall target) Now, all builtin polyfills are resolved from the project root
  • Is it a concern that packageManager.resolve is slow? I suspect there won't be that many Node builtins anyway

@@ -54,6 +54,7 @@
"mocha-junit-reporter": "^2.0.0",
"mocha-multi-reporters": "^1.5.1",
"prettier": "2.4.1",
"punycode": "^1.4.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

The url polyfill package declares punycode as a dependency

https://github.com/defunctzombie/node-url/blob/5480ec001102457ac00c3c7878facdec39b536c8/package.json#L6-L9

but then does require("punycode") which uses the node polyfill (as opposed to require("punycode/")). I needed to declare this version to get the integration tests to pass....

@devongovett devongovett merged commit 527e477 into v2 Sep 3, 2022
@devongovett devongovett deleted the node-builtins-monorepo branch September 3, 2022 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants