-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix: no longer warns when no name was provided for global module #2359
Conversation
98ff468
to
d4267ef
Compare
d4267ef
to
ce856cf
Compare
ce856cf
to
2548760
Compare
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.
Thanks for the PR! This is an important fix.
I've provided a suggestion around trying to isolate your use case from more simple workflows. Let me know your thoughts.
|
||
if (globalName) { | ||
return globalName; | ||
} |
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 think it's ok to not warn when the user has not specified any configuration, as in many cases this is the required behaviour that the package name be the request package name (eg this is the default on npm that a request package name and output package name can match).
The issue maybe arises in environments in which you are defining a global function or map that is trying to be authoritative and then has these invisible misses.
Instead, perhaps let's only warn for the case where globals
is provided but has misses, by adding that as another case above?
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 am not 100% sure about this, as for UMD, I noticed that a lot of guessed name would be incorrect. If for instance having a .
in the name. Or when the umd bundle name is totally different, and without a warning, it's hard to rollup users to know that something might be wrong.
However I can do what you suggested.
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.
@guybedford pushed an update.
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.
Seems like a worthy regression fix to me.
2707448
to
69c9da2
Compare
src/Chunk.ts
Outdated
return globalName; | ||
} | ||
|
||
if (hasExports || (hasExports ** globals && !globalName)) { |
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.
Did you mean &&
here?
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.
yeah looks like the IDE didn't save :P, re-pushed. Thanks for your patience 👍
69c9da2
to
10cd889
Compare
src/Chunk.ts
Outdated
return globalName; | ||
} | ||
|
||
if (hasExports || (hasExports && globals && !globalName)) { |
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 (hasExports || globals && !globalName)
would be simpler here I think.
I've been thinking about this some more and maybe you are right we should always warn actually... |
10cd889
to
2548760
Compare
hahah, removed the last commit :-)
…On Mon, 30 Jul 2018 at 14:13, Guy Bedford ***@***.***> wrote:
I've been thinking about this some more and maybe you are right we should
always warn actually...
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2359 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AQv-WkwKWGdbBjdQnrha1iJ0YMqxPsogks5uLvhjgaJpZM4VlBf1>
.
|
LGTM |
When the dictionary lookup or function look up return undefined or null, a warning should be emitted and the module name should be guessed.
If the user, doesn't receive a warning that the global name was not found, they won't be able to fix a possible issue. If the global name is incorrect or later set to null.
Closes: #2358
Some more context
ng-packagr/ng-packagr#1026 (comment)