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

chore: replace resolve with create-require #12396

Closed
wants to merge 2 commits into from

Conversation

merceyz
Copy link
Contributor

@merceyz merceyz commented Nov 25, 2020

Q                       A
Fixed Issues?
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? TBD
Documentation PR Link
Any Dependency Changes? Replaced resolve with create-require
License MIT

Babel seems to only need resolve because of its basedir option, however Node supports using require.resolve(id, { paths: [baseDir] }) or createRequire*(baseDir).resolve(id) for this. Since Babel still supports Node 6, which doesn't have any of these, a polyfill is used for createRequire.

create-require can be removed in Babel 8 in favour of just using require.resolve

The benefit of this change is that PnP support is preserved if someone feels like bundling Babel (vercel/next.js#18768)

@babel-bot
Copy link
Collaborator

babel-bot commented Nov 25, 2020

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/33244/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 25, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 860dc83:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

@JLHwung
Copy link
Contributor

JLHwung commented Nov 25, 2020

/cc @ljharb.

@ljharb
Copy link
Member

ljharb commented Nov 25, 2020

resolve already has PnP support. What motivates this change?

resolve has 37 million weekly downloads, create-require has 200K - it seems much safer for babel to rely on the defacto standard in the industry for the time being.

@merceyz
Copy link
Contributor Author

merceyz commented Nov 25, 2020

resolve already has PnP support. What motivates this change?

Only when patched by Yarn at install time, which is not possible when bundled (as mentioned in the OP) since Yarn can't detect it.

resolve has 37 million weekly downloads, create-require has 200K - it seems much safer for babel to rely on the defacto standard in the industry for the time being.

The only part from it that is needed is the polyfill https://github.com/nuxt-contrib/create-require/blob/f45cb926b1a6cffb1b879377933155c7ac523a76/create-require.js#L30-L37 which i'm perfectly fine with inlining until it can be removed in Babel 8.

The full version when inlined would look like this one from gatsby https://github.com/gatsbyjs/gatsby/blob/3e9279563f76cef4e6a25ccb52aef85b5f6c2b6d/packages/gatsby-core-utils/src/create-require-from-path.ts

@merceyz
Copy link
Contributor Author

merceyz commented Nov 25, 2020

While I have you here @ljharb, seems resolve isn't following symlinks as aggressively as node based on the changes in the fixtures

@nicolo-ribaudo
Copy link
Member

In the next-8-dev branch, we are using require.resolve without errors: #11125

@ljharb
Copy link
Member

ljharb commented Nov 25, 2020

@merceyz resolve v1 has preserveSymlinks set to true, to avoid a breaking change, which was the original node behavior. the prereleases for resolve v2 have it default to false, matching modern node's behavior.

It would be a great idea for babel to upgrade to using v2 of resolve, if possible.

@merceyz
Copy link
Contributor Author

merceyz commented Nov 26, 2020

In the next-8-dev branch, we are using require.resolve without errors: #11125

Even better

@merceyz resolve v1 has preserveSymlinks set to true, to avoid a breaking change, which was the original node behavior. the prereleases for resolve v2 have it default to false, matching modern node's behavior.

Ah, my bad, was looking at the readme for the master branch (v2).

It would be a great idea for babel to upgrade to using v2 of resolve, if possible.

Using the builtin require seems safer to me and solves the main reason I opened this PR. (PnP support when babel and its dependencies are bundled)

@ljharb
Copy link
Member

ljharb commented Nov 26, 2020

regular require also won’t work any better than resolve if pnp isn’t able to interject - which it can with both, and I’d assume is what gets written into the bundle.

@merceyz
Copy link
Contributor Author

merceyz commented Nov 26, 2020

regular require also won’t work any better than resolve if pnp isn’t able to interject

PnP is always able to control the builtin require, it always goes through Module._resolveFilename which the PnP hook controls

which it can with both, and I’d assume is what gets written into the bundle.

Assuming the people making the bundle runs under Yarn 2 then sure, a bundled resolve will have PnP support, but in this case they aren't and so it doesn't have PnP support.

@ljharb
Copy link
Member

ljharb commented Nov 27, 2020

This would still be true with require or require.resolve tho - the results of those get written into the bundle (bundles primarily being for the web, where no filesystem exists). If you make a node bundle, then it’d still be the native require which pnp can hook into.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Dec 1, 2020

bundles primarily being for the web, where no filesystem exists

The problem is specifically for bundles targeting Node.js (e.g. Next.js's build system is bundled, including Babel). require.resolve still delegates to the filesystem to resolve additional Babel plugins specified by the user.

Anyway, we are already removing require in Babel 8 since require.resolve(..., { paths: [basedir] }) is enough for what Babel needs. If we need this PR to make Next.js support Yarn PnP, I would be ok with start using now require.resolve when it's supported (Node.js >= 8.9) and use a small polyfill for Node.js 6.

However, I'd like to propose an alternative PR: main...nicolo-ribaudo:require-resolve-node-6

By doing so,

  • we avoid the new dependency
  • we only have to delete that Babel plugin in Babel 8
  • we use the native require.resolve when it supports the paths option.

Note that customizing our build process to support older Node.js versions is something we already did: for example, before #12288 we were using a custom plugin to fix resolution of dynamic import paths in some Node.js versions.

@merceyz
Copy link
Contributor Author

merceyz commented Dec 1, 2020

Your alternative is fantastic, it gets the job done and it's easy to remove the polyfill when Babel 8 rolls around so i'm all for it 👍

@ljharb

This comment has been minimized.

@nicolo-ribaudo
Copy link
Member

We only use it for resolving plugins and presets, for transpiling we use glob (or something similar, I don't remember).

@nicolo-ribaudo nicolo-ribaudo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Dec 2, 2020
@merceyz merceyz deleted the merceyz/resolve branch December 4, 2020 08:06
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Mar 6, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants