Skip to content

Verify version when resolving Node builtin polyfills#8387

Merged
devongovett merged 9 commits into
v2from
node-builtins-monorepo
Sep 3, 2022
Merged

Verify version when resolving Node builtin polyfills#8387
devongovett merged 9 commits into
v2from
node-builtins-monorepo

Conversation

@mischnic

@mischnic mischnic commented Aug 10, 2022

Copy link
Copy Markdown
Member

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

@mischnic mischnic force-pushed the node-builtins-monorepo branch from 6092f6a to 69ef97a Compare August 10, 2022 09:44
@mischnic mischnic requested a review from devongovett August 11, 2022 21:12
@mischnic mischnic force-pushed the node-builtins-monorepo branch from 69ef97a to 02290e2 Compare August 24, 2022 17:39
Comment thread packages/utils/node-resolver-core/src/builtins.js Outdated
@mischnic mischnic force-pushed the node-builtins-monorepo branch from 65884f9 to 4ecff55 Compare August 31, 2022 08:28
Comment thread package.json
"mocha-junit-reporter": "^2.0.0",
"mocha-multi-reporters": "^1.5.1",
"prettier": "2.4.1",
"punycode": "^1.4.1",

Copy link
Copy Markdown
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.

2 participants