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

add 'includeCoreModules' option #233

Merged
merged 1 commit into from Nov 10, 2020

Conversation

tjenkinson
Copy link
Contributor

Setting includeCoreModules: false makes it possible to resolve as if the core modules did not exist.

This can be useful in a bundler like rollup where the user may be targeting web, where node builtins are not relevant.

Refs rollup/plugins#637

@ljharb
Copy link
Member

ljharb commented Nov 9, 2020

This has usually been achieved via https://www.npmjs.com/package/browser-resolve - it seems much better for rollup to just use that package rather than adding complexity to resolve and reinventing decade-built wheels?

@tjenkinson
Copy link
Contributor Author

tjenkinson commented Nov 9, 2020

Hey @ljharb !

Thanks for pointing out browser-resolve. Agree it would be best not to duplicate logic where possible.

I think this PR is still useful though and enables a feature I think browser-resolve currently doesn't have.

If I'm bundling for web, I want to guarantee that core modules aren't used. I think with browser-resolve you'd have to provide something for all the builtins on modules?

Also if I import fs I'd like that to load the node_modules version automatically without having to make it fs/.

@ljharb
Copy link
Member

ljharb commented Nov 9, 2020

Yes, any bundler for node modules must provide shims for node core modules, otherwise it's broken (like webpack 5 is currently broken, for example). That allows, for example, importing fs to transparently map to the shim (not that most fs things are shimmable in browsers, ofc) without the trailing slash. This is what browser-resolve does.

If you're trying to deny access to a core module that could be shimmed in a browser (which, for a node module bundler, would mean it's broken) you should already be able to do this in resolve via a combination of callback options, which is how browser-resolve does it.

@tjenkinson
Copy link
Contributor Author

If you're trying to deny access to a core module that could be shimmed in a browser (which, for a node module bundler, would mean it's broken) you should already be able to do this in resolve via a combination of callback options, which is how browser-resolve does it.

Yeh this is what I meant.

If you do

browserResolve.sync('fs')

This returns fs, when I think a module not found error would be preferable, or the absolute path to the fs package in node_modules if there was one.

@ljharb
Copy link
Member

ljharb commented Nov 9, 2020

hmm, interesting. I do see how resolve simply doesn't permit you to override the "isCore" logic, and always returns core module names directly.

This seems reasonable to add, thanks for bearing with me.

@ljharb ljharb force-pushed the include-core-modules-option branch 2 times, most recently from 9d308e3 to 7f5a63b Compare November 10, 2020 00:32
@ljharb ljharb merged commit 7f5a63b into browserify:master Nov 10, 2020
@tjenkinson
Copy link
Contributor Author

Thanks!

ljharb added a commit that referenced this pull request Nov 10, 2020
 - [New] `sync`/`async`: add 'includeCoreModules' option (#233)
 - [readme] Add possible error types (#232)
 - [Deps] update `is-core-module`
 - [Dev Deps] update `aud`, `eslint`
 - [meta] add Automatic Rebase and Require Allow Edits workflows
 - [Tests] comment out node 15 in appveyor; it’s not available yet
 - [Tests] add node 15 to appveyor, fix "latest npm" logic
 - [Tests] migrate tests to Github Actions
ljharb added a commit that referenced this pull request Nov 10, 2020
v1.19.0

 - [New] `sync`/`async`: add 'includeCoreModules' option (#233)
 - [readme] Add possible error types (#232)
 - [Deps] update `is-core-module`
 - [Dev Deps] update `aud`, `eslint`
 - [meta] add Automatic Rebase and Require Allow Edits workflows
 - [Tests] comment out node 15 in appveyor; it’s not available yet
 - [Tests] add node 15 to appveyor, fix "latest npm" logic
 - [Tests] migrate tests to Github Actions
@ljharb
Copy link
Member

ljharb commented Nov 10, 2020

Released in v1.19.0; it'll also be included in the next v2 prerelease.

@ljharb ljharb mentioned this pull request Dec 1, 2020
ljharb added a commit to ljharb/resolve that referenced this pull request Feb 11, 2021
Changes since v2.0.0-next.2:

 - [New] add `readPackage` and `readPackageSync` (browserify#236)
 - [Fix] throw an error for an invalid explicit `main` with a missing `./index.js` file (browserify#234)
 - [meta] create SECURITY.md
 - [Deps] update `is-core-module`
 - [Dev Deps] update `eslint`, `@ljharb/eslitn-config`, `array.prototype.map`, `aud`, `tape`

Including all changes in v1.15.1 - v1.19.0:

v1.19.0

 - [New] `sync`/`async`: add 'includeCoreModules' option (browserify#233)
 - [readme] Add possible error types (browserify#232)
 - [Deps] update `is-core-module`
 - [Dev Deps] update `aud`, `eslint`
 - [meta] add Automatic Rebase and Require Allow Edits workflows
 - [Tests] comment out node 15 in appveyor; it’s not available yet
 - [Tests] add node 15 to appveyor, fix "latest npm" logic
 - [Tests] migrate tests to Github Actions

v1.18.1

 - [Fix] `core`: remove console warning on require, since the main entry point requires it
 - [Refactor] avoid using extensions in require paths
 - [meta] update auto-generated `core.json`
 - [meta] add `eclint` and `.editorconfig`
 - [Dev Deps] update `eslint`
 - [Dev Deps] add `aud` in `posttest`

v1.18.0

 - [New] extract `isCore` implementation to `is-core-module`
 - [New] add new core modules that will be in node v15
 - [readme] soft-deprecate `resolve.isCore`
 - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `tape`

v1.17.0

 - [New] `sync`/`async`: add `realpath`/`realpathSync` options (browserify#218)
 - [Dev Deps] update `tape`

v1.16.1

 - [patch] when a non-node `fs` is broken and lacks `realpath`/`realpathSync`, do not crash (browserify#220)

v1.16.0

 - [New] `core`: `fs/promises` is a core module again in node 14+ (f6473e2)
 - [patch] `sync`/`async`: use native `realpath` if available to unwrap symlinks (browserify#217)

v1.15.1

 - [Fix] correct behavior when requiring `.` with same name (browserify#212)
 - [Dev Deps] update `@ljharb/eslint-config`
 - [Tests] allow node 5 on windows to fail due to npm registry bug
ljharb added a commit that referenced this pull request Feb 11, 2021
Changes since v2.0.0-next.2:

 - [New] add `readPackage` and `readPackageSync` (#236)
 - [Fix] throw an error for an invalid explicit `main` with a missing `./index.js` file (#234)
 - [meta] create SECURITY.md
 - [meta] do not publish github action workflow files
 - [Deps] update `is-core-module`
 - [Dev Deps] update `eslint`, `@ljharb/eslitn-config`, `array.prototype.map`, `aud`, `tape`

Including all changes in v1.15.1 - v1.19.0:

v1.19.0

 - [New] `sync`/`async`: add 'includeCoreModules' option (#233)
 - [readme] Add possible error types (#232)
 - [Deps] update `is-core-module`
 - [Dev Deps] update `aud`, `eslint`
 - [meta] add Automatic Rebase and Require Allow Edits workflows
 - [Tests] comment out node 15 in appveyor; it’s not available yet
 - [Tests] add node 15 to appveyor, fix "latest npm" logic
 - [Tests] migrate tests to Github Actions

v1.18.1

 - [Fix] `core`: remove console warning on require, since the main entry point requires it
 - [Refactor] avoid using extensions in require paths
 - [meta] update auto-generated `core.json`
 - [meta] add `eclint` and `.editorconfig`
 - [Dev Deps] update `eslint`
 - [Dev Deps] add `aud` in `posttest`

v1.18.0

 - [New] extract `isCore` implementation to `is-core-module`
 - [New] add new core modules that will be in node v15
 - [readme] soft-deprecate `resolve.isCore`
 - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `tape`

v1.17.0

 - [New] `sync`/`async`: add `realpath`/`realpathSync` options (#218)
 - [Dev Deps] update `tape`

v1.16.1

 - [patch] when a non-node `fs` is broken and lacks `realpath`/`realpathSync`, do not crash (#220)

v1.16.0

 - [New] `core`: `fs/promises` is a core module again in node 14+ (f6473e2)
 - [patch] `sync`/`async`: use native `realpath` if available to unwrap symlinks (#217)

v1.15.1

 - [Fix] correct behavior when requiring `.` with same name (#212)
 - [Dev Deps] update `@ljharb/eslint-config`
 - [Tests] allow node 5 on windows to fail due to npm registry bug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants