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
feat(node-resolve): Mark built-ins as external #627
Merged
Merged
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
39c9ce1
do not ignore errors
tjenkinson b456520
do not error when import specifier does not map to file
tjenkinson d78fa0f
always swallow MODULE_NOT_FOUND errors
tjenkinson 81890b7
return null from resolveId early if can't resolve
tjenkinson edffa14
strip null byte from importer if there is one
tjenkinson b4daacd
fix root-dir test
tjenkinson b321a7d
remove try/catch
tjenkinson 703f9cd
treat builtins as externals (depending on useBuiltins option)
tjenkinson 512b06e
update readme sections on builtins
tjenkinson 8ffc867
fix readme doc about `jail`
tjenkinson 77860fb
update all other snapshots
tjenkinson 9216024
use exists() instead of try/catch
tjenkinson 1f43a90
refactor resolveImportSpecifiers to use async/await
tjenkinson 3a26b3a
Merge remote-tracking branch 'upstream/master' into external-builtins
tjenkinson 3eab8f6
use async `realpath`
tjenkinson aebc3e5
remove duplicate lines
tjenkinson f6c1ec4
remove readme update
tjenkinson 197be11
fix doc
tjenkinson 00673c4
add note that `preferBuiltins` should be `false` for stubbed builtins
tjenkinson 166c024
Merge remote-tracking branch 'upstream/master' into external-builtins
tjenkinson File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file modified
BIN
-321 Bytes
(49%)
packages/node-resolve/test/snapshots/prefer-builtins.js.snap
Binary file not shown.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 likefs
. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
orfalse
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
I'd be happy for it to be an explicit option. Are you suggesting that
unset
should becomeauto
? or something else?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your example looks very much like the
default
forpreferBuiltins
would betrue
, just with a warning about it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh removing this comment. Opened another PR to simplify how we handle
preferBuiltins
a bit#637