feat(node-resolve): Mark built-ins as external #627
Conversation
as not being able to resolve the id is a valid output
this was passing before because the real error was being ignored
packages/node-resolve/README.md
Outdated
|
|
||
| If `true`, the plugin will prefer built-in modules (e.g. `fs`, `path`). If `false`, the plugin will look for locally installed modules of the same name. | ||
|
|
||
| If unset the plugin will first look for modules of the same name, and then fall back to built-in modules. |
There was a problem hiding this comment.
If we keep the warning about unset that still appears to be in the code, I guess this should be mentioned here, otherwise people think they can get away with not configuring this option when they use imports like fs. Alternatively, we could allow this as a valid "third" option, e.g. "auto", where you want to override some builtins with local alternatives but keep others. But I am not sure I can imagine a scenario where people would want this.
There was a problem hiding this comment.
The plugin behaves a bit differently when it's unset. I was confused by this because I though it would default to either the true or false behaviour, but it's essentially a third option, so this was to try and make that clear.
The way these 2 variables are used makes the third behaviour
const isPreferBuiltinsSet = options.preferBuiltins === true || options.preferBuiltins === false;
const preferBuiltins = isPreferBuiltinsSet ? options.preferBuiltins : true;I'd be happy for it to be an explicit option. Are you suggesting that unset should become auto? or something else?
There was a problem hiding this comment.
Looking at the code again now I'm not actually sure if the line I added here is correct. It's quite confusing to follow through the logic
There was a problem hiding this comment.
Your example looks very much like the default for preferBuiltins would be true, just with a warning about it.
There was a problem hiding this comment.
Yeh removing this comment. Opened another PR to simplify how we handle preferBuiltins a bit
shellscape
changed the title
|
Thanks for the PR. It looks like there are some tests breaking in CI that will need to be addressed, and we'll need to know if these changes proposed will be breaking changes for users (you didn't select an option in the template) |
It looks like this was just a failure to update coverage info? A rerun might fix that.
Yeh I wasn't 100% on this so I left a comment above the check boxes. I guess if we're unsure it's probably safest to go with 'yes' |
|
I have some more changes coming up in a different PR that would need to be major, so if you would prefer to release as few major bumps as possible, it might be best to wait a few days for that PR too. |
|
Here's the other PR which would need to be major: #656 |
|
|
Rollup Plugin Name:
node-resolveThis PR contains:
Are tests included?
Breaking Changes?
Not sure on this given previously it was a warning. It would have allowed other plugins to jump in though so maybe it should be 'yes'?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
Description
This continues from #564.
I thought that if
useBuiltinswastruethen any builtins would automatically be marked as external. This was not the case. This PR does that. Let me know if you think it doesn't make sense.It also makes some of the docs a bit more accurate.
It seems a bit strange that some options (like
jail) don't mark an import as external, but others (likeresolveOnlyand nowpreferBuiltins) do. I wonder if there is a reason or if we should make them consistent?There is one commit that updates all the snapshots as I noticed when updating the ones for the tests I updated it changed them all because the ava domain in the comment changed.