-
Notifications
You must be signed in to change notification settings - Fork 204
Feat: warn when the package from resolved alias is not available #160
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
Changes from 16 commits
501b839
fa481cd
3e15eec
b42ceb2
c55a2d0
fda055b
024dcb1
81d528d
da1fcd9
1b4f87f
c59bcee
040bb1c
7c71d3e
ce36607
5b1b900
3caaaa1
c0baaa0
66bc9c6
dc01d6d
8e55566
647ce41
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| // This module exists only for abstracting logging away and making testing easier | ||
|
|
||
| // eslint-disable-next-line import/prefer-default-export | ||
| export function warn(...args) { | ||
| // eslint-disable-next-line no-console | ||
| console.warn(...args); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,29 +51,42 @@ function normalizeRoot(opts) { | |
| } | ||
| } | ||
|
|
||
| function normalizeAlias(opts) { | ||
| opts.regExps = []; | ||
|
|
||
| if (opts.alias) { | ||
| Object.keys(opts.alias) | ||
| .filter(isRegExp) | ||
| .forEach((key) => { | ||
| const parts = opts.alias[key].split('\\\\'); | ||
|
|
||
| function substitute(execResult) { | ||
| return parts | ||
| .map(part => | ||
| part.replace(/\\\d+/g, number => execResult[number.slice(1)] || ''), | ||
| ) | ||
| .join('\\'); | ||
| } | ||
| function getAliasPair(key, value) { | ||
| const parts = value.split('\\\\'); | ||
|
|
||
| function substitute(execResult) { | ||
| return parts | ||
| .map(part => | ||
| part.replace(/\\\d+/g, number => execResult[number.slice(1)] || ''), | ||
| ) | ||
| .join('\\'); | ||
| } | ||
|
|
||
| opts.regExps.push([new RegExp(key), substitute]); | ||
| return [new RegExp(key), substitute]; | ||
| } | ||
|
|
||
| delete opts.alias[key]; | ||
| }); | ||
| function normalizeAlias(opts) { | ||
| if (opts.alias) { | ||
| const { alias } = opts; | ||
| const aliasKeys = Object.keys(alias); | ||
| const nonRegExpAliases = []; | ||
| const regExpAliases = []; | ||
|
|
||
| aliasKeys.forEach((key) => { | ||
| if (isRegExp(key)) { | ||
| regExpAliases.push( | ||
| getAliasPair(key, alias[key]), | ||
| ); | ||
| } else { | ||
| nonRegExpAliases.push( | ||
| getAliasPair(`^${key}((?:/|).*)`, `${alias[key]}\\1`), | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| opts.alias = [...nonRegExpAliases, ...regExpAliases]; | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think of this? opts.alias = aliasKeys.reduce((acc, key) => {
acc.push(
isRegExp(key)
? getAliasPair(key, alias[key])
: getAliasPair(`^${key}((?:/|).*)`, `${alias[key]}\\1`)
);
return acc;
}, []);
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to preserve the order: standard aliases before regexps. At this point I think it could go either way, as the order should be taken from the object. Note: according to the spec, the order of enumerating the object keys is not guaranteed to be the same as in the literal, so in the future it may be needed to allow an array-type declaration. We're currently relying on V8.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, one or the other, we'll have an issue if a user expects the order to be kept. With an object, it's not guaranteed as you say. We'll figure out a way later if really needed. For now, it doesn't seem like users need it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll go with the single array then.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (btw.
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed. |
||
| } else { | ||
| opts.alias = {}; | ||
| opts.alias = []; | ||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
I missed that, but actually it should be the custom extensions the user sets, or the babel extension.
so
extensionsfromopts.And we should reuse this function in
findPathInRootsto remove the duplicate.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.
I'm not sure about this one - the packages are 3rd party and so they will probably have standard extensions.
OTOH it shouldn't hurt much to include more files in the lookup, as long as they are package-ish (what should be ensured by
require.resolve). In the worst case there will be false positives, which would result in some warnings being silenced, but it doesn't sound very bad.I'll go with your suggested change.
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.
Hmm.. Maybe I missed a detail, but it was used in the alias config to test the alias was actually pointing to an existing file (or node_modules). By 3rd party, you mean a node_module package, right?
If we really want to be safe, we could use the user/babel extensions for local files/modules and then the node extensions for the ones under node_modules (the ones who don't start with ./). But would it be really useful? Doesn't seem like it's really worth it to me. Again, we'll iterate if really needed, otherwise I'm ok with potential false positives. Anyway, this is only a helper for the user. We never checked the alias was correct in any previous versions so it's already a bit better now :)
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.
Yes, a
node_modulespackage. And yes, let's just see if anyone complains at all :)