node-resolve: simplify builtins and remove `customResolveOptions` #656

tjenkinson · 2020-11-21T16:16:04Z

Rollup Plugin Name: `node-resolve`

This PR contains:

Are tests included?

yes
no

Breaking Changes?

yes
no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

Description

Note this includes #637

This PR upgrades resolve to the next version which includes the includeCoreModules option.

We are no longer relying on our builtins list to agree with resolve's. If we decide it's a builtin, it's a builtin, otherwise we hand off to resolve which will never treat it as a builtin. This meant we could remove a block of code that was there just to make sure resolve didn't treat something as a builtin if it had the same name.

It also makes some other general changes such as moving the use of resolve and the filter for resolve into resolveImportSpecifiers and returning

{
  location,
  hasModuleSideEffects,
  hasPackageEntry,
  packageBrowserField,
  packageInfo
}

from this function instead of just the location. Previously the resolve options were passed down to resolveImportSpecifiers() and the filter was in index, along with the other variables which were updated as a side effect of the filter running.

There should be no functional changes, other than the removal of customResolveOptions mentioned below.

Breaking changes

I removed customResolveOptions and added moduleDirectories (which used to be customResolveOptions.moduleDirectory). My reasoning for this was

so that we aren't coupling the api of node-resolve to resolve
because if a user replaced some of the resolve options it would actually break the plugin. If the user added their own filter for example ours wouldn't run
Overriding an option like resolveSymlinks doesn't make sense IMO when rollup itself has the same option, and we also inherit from that anyway.

I'm not sure what other options other than moudleDirectory users might want to use, but I think if it turns out a user is using one of the other options, we could add it back as a top level option.

Also I removed the only option, which was already marked as deprecated.

…resolve-refactor-builtins

this also removes `customResolveOptions` and adds `moduleDirectory`

lukastaegert

Code-wise, I think this looks good, thanks for taking this up! I had some comments mostly with regard to documentation, would be nice if you could use this PR to also make sure documentation, types and code are in sync.

lukastaegert · 2020-11-22T06:10:29Z

.eslintrc.js

@@ -33,6 +33,7 @@ module.exports = {
    'import/no-namespace': 'off',
    'import/no-named-export': 'off',
    'no-unused-vars': 'off',
+    'spaced-comment': 'off',


Is it really necessary to mess with global linting rules? If there is really a good reason to ignore it at some point, rather use a // eslint-disable-next-line spaced-comment.

There seems to be a bug with this rule and '.d.ts' files. I tried a lot of things to make it work, including

/* eslint-disable spaced-comment */

in the file but it seems to ignore it :/

Whenever it tries to 'fix' the lint errors it ends up inserting random spaces everywhere and then still fails for a different reason.

If it only affects d.ts files, then I would recommend adding it as an override:

overrides: [ files: ['**/*.d.ts'], rules: [{'spaced-comment' : 'off'}] ]

lukastaegert · 2020-11-22T06:15:18Z

packages/node-resolve/README.md

-  moduleDirectory: 'js_modules'
-}
-```
+Directory in which to recursively look for modules.


Is there still a way to modify the preserveSymlinks option? Of all remaining resolve options, I think this one is really important as some people with uncommon package systems rely on it.

Ok, I guess the option still exists in the code but is not documented. Do you think you might add it here, because I fear for lack of documentation, some people might have used the customResolveOptions for that.

Yeh it always inherits the rollup one now. I added this to the readme.

packages/node-resolve/test/types.ts

LarsDenBakker · 2020-11-22T10:21:16Z

packages/node-resolve/README.md

@@ -62,18 +62,12 @@ Default: `false`

 If `true`, instructs the plugin to use the `"browser"` property in `package.json` files to specify alternative files to load for bundling. This is useful when bundling for a browser environment. Alternatively, a value of `'browser'` can be added to the `mainFields` option. If `false`, any `"browser"` properties in package files will be ignored. This option takes precedence over `mainFields`.

-### `customResolveOptions`
+### `moduleDirectory`


Despite the singular name, moduleDirectory can actually be an array in resolve. We use this extensively as we have some modules installed via bower, so we set this to `['node_modules', 'bower_components'].

Thanks. Will make this clear

pushed commit for this

LarsDenBakker · 2020-11-23T06:34:41Z

Would be it be worth doing some backwards compatibility work by reading the old customResolveOptions object and mapping them to the new properties? Lately rollup plugins have a breaking change every other week, which places extra burden on consumers to keep up.

If that's not possible we might need to consider saving up this breaking change, since breaking for a refactor isn't great.

tjenkinson · 2020-11-23T09:01:36Z

Would be it be worth doing some backwards compatibility work by reading the old customResolveOptions object and mapping them to the new properties? Lately rollup plugins have a breaking change every other week, which places extra burden on consumers to keep up.

Was wondering this. Happy to add aliases and a warning for the options that still exist.

If that's not possible we might need to consider saving up this breaking change, since breaking for a refactor isn't great.

There is already another major change ready to go: #627

Which I was hoping we could batch together.

lukastaegert · 2020-11-23T09:21:35Z

I guess it still makes sense to alias the old options to reduce friction. In that case, we should also have a warning that instructs people to use the new options.

tjenkinson · 2020-11-23T21:10:11Z

Added a deprecation warning for customResolveOptions.moduleDirectory and made it an alias for moduleDirectories.

For the rest of the options it throws an error now.

…lify-builtins

looks like auto lint messed up

…ptions` (#656) BREAKING CHANGES: See rollup/plugins#656 * refactor handling builtins * remove duplicate code block * do not warn when using a builtin when no local version * add not about warnings to readme * update node-resolve, use `includeCoreModules`, and simplify this also removes `customResolveOptions` and adds `moduleDirectory` * remove console log * remove deprecated `only` option * remove `only` from types * update types test * lint index.d.ts * disable spaced-comment rule because it's breaking on .d.ts files * mention rollup `preserveSymlinks` option in readme * moduleDirectory => moduleDirectories * put filter in config object directly * add missing options to types.ts * add deprecation warnings/errors * handle deprecations in separate file * move catch closer to error * revert commonjs changes looks like auto lint messed up * remove old comment Co-authored-by: Andrew Powell <shellscape@users.noreply.github.com>

tjenkinson added 12 commits November 8, 2020 11:36

refactor handling builtins

dc2deca

Merge branch 'master' of https://github.com/rollup/plugins into node-…

74f76ec

…resolve-refactor-builtins

remove duplicate code block

b19fb3e

do not warn when using a builtin when no local version

15302c6

add not about warnings to readme

44d0625

update node-resolve, use includeCoreModules, and simplify

8158663

this also removes `customResolveOptions` and adds `moduleDirectory`

remove console log

83cacfb

remove deprecated only option

56e3e0b

remove only from types

313d9f0

update types test

3cfdf68

lint index.d.ts

a7b1603

disable spaced-comment rule because it's breaking on .d.ts files

5aa675e

tjenkinson marked this pull request as ready for review November 21, 2020 16:42

tjenkinson mentioned this pull request Nov 21, 2020

feat(node-resolve): Mark built-ins as external #627

Merged

9 tasks

lukastaegert reviewed Nov 22, 2020

View reviewed changes

LarsDenBakker reviewed Nov 22, 2020

View reviewed changes

tjenkinson added 4 commits November 22, 2020 10:55

mention rollup preserveSymlinks option in readme

8b3d895

moduleDirectory => moduleDirectories

97a4dc1

put filter in config object directly

3033e93

add missing options to types.ts

3c8cec4

add deprecation warnings/errors

12eca25

handle deprecations in separate file

486150e

shellscape added pr⁶ 🔧 refactor 🔌 plugin-node-resolve labels Nov 28, 2020

tjenkinson added 2 commits November 28, 2020 10:55

Merge remote-tracking branch 'upstream/master' into node-resolve-simp…

89778dd

…lify-builtins

move catch closer to error

8d39f74

tjenkinson and others added 2 commits November 28, 2020 11:02

revert commonjs changes

e4eb3e2

looks like auto lint messed up

remove old comment

2684129

lukastaegert approved these changes Nov 30, 2020

View reviewed changes

Merge branch 'master' into node-resolve-simplify-builtins

2ec4668

shellscape merged commit 23b0bf7 into rollup:master Nov 30, 2020

bbmarkus mentioned this pull request Jan 7, 2021

[node-resolve] customResolveOptions.filterPackage alternative wanted #761

Closed

hsubox76 mentioned this pull request Feb 24, 2021

Update @rollup/plugin-node-resolve firebase/firebase-js-sdk#4518

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node-resolve: simplify builtins and remove `customResolveOptions` #656

node-resolve: simplify builtins and remove `customResolveOptions` #656

tjenkinson commented Nov 21, 2020 •

edited

lukastaegert left a comment

lukastaegert Nov 22, 2020

tjenkinson Nov 22, 2020

lukastaegert Nov 30, 2020 •

edited

lukastaegert Nov 22, 2020

lukastaegert Nov 22, 2020

tjenkinson Nov 22, 2020

LarsDenBakker Nov 22, 2020

tjenkinson Nov 22, 2020

tjenkinson Nov 22, 2020

LarsDenBakker commented Nov 23, 2020 •

edited

tjenkinson commented Nov 23, 2020

lukastaegert commented Nov 23, 2020

tjenkinson commented Nov 23, 2020

node-resolve: simplify builtins and remove customResolveOptions #656

node-resolve: simplify builtins and remove customResolveOptions #656

Conversation

tjenkinson commented Nov 21, 2020 • edited

Rollup Plugin Name: node-resolve

Description

Breaking changes

lukastaegert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukastaegert Nov 30, 2020 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LarsDenBakker commented Nov 23, 2020 • edited

tjenkinson commented Nov 23, 2020

lukastaegert commented Nov 23, 2020

tjenkinson commented Nov 23, 2020

node-resolve: simplify builtins and remove `customResolveOptions` #656

node-resolve: simplify builtins and remove `customResolveOptions` #656

tjenkinson commented Nov 21, 2020 •

edited

Rollup Plugin Name: `node-resolve`

lukastaegert Nov 30, 2020 •

edited

LarsDenBakker commented Nov 23, 2020 •

edited