build: add tslint rule to verify rollup globals config #5963
Conversation
googlebot
added
the
cla: yes
crisbeto
force-pushed
the
rollup-globals-rule
branch
from
4a24929 to
f769f72
Compare
devversion
left a comment
devversion
left a comment
There was a problem hiding this comment.
Good idea. I like that the globals are in a separate file.
| const module = node.moduleSpecifier.getText().slice(1, -1); | ||
|
|
||
| // Check whether we match any of the scopes while missing the module from the config. | ||
| if (this._scope.some(p => p.test(module)) && !this._config[module]) { |
There was a problem hiding this comment.
More like a question. Why do we need such a scope field? Actually every package import should be listed in the globals config?
There was a problem hiding this comment.
It's primarily because we've only had issues with Angular and Rx packages until now since they're the only dependencies that we have. I can switch it to look for all external packages instead, if it makes sense.
There was a problem hiding this comment.
I see what you mean, but Rollup will not only complain about the dependencies/scopes you restricted the rule to. Basically every external import (that is not added to that list) will be problematic
There was a problem hiding this comment.
Makes sense, I'll change it. While we're at it, I'm not too sure how to handle the @angular/material-examples package. Currently I've added an exception for it.
There was a problem hiding this comment.
Can you explain what you mean? What's special for the examples package?
There was a problem hiding this comment.
What is the proper Rollup mapping for it? E.g. for @angular/core it's ng.core, but I'm not sure whether the examples have something like that.
There was a problem hiding this comment.
Yeah that doesn't really matter that much since it won't be really a public package.
As for convention I'd map it to something like ng.material-examples.
There was a problem hiding this comment.
Done. Also made it resolve the path relative to the project root, instead of the rule file.
crisbeto
force-pushed
the
rollup-globals-rule
branch
2 times, most recently
from
e724403 to
6bf2ff1
Compare
devversion
added
pr: lgtm
action: merge
crisbeto
force-pushed
the
rollup-globals-rule
branch
from
6bf2ff1 to
dd82287
Compare
|
Please rebase |
crisbeto
force-pushed
the
rollup-globals-rule
branch
from
dd82287 to
5c046de
Compare
|
Rebased @andrewseguin. |
|
Got some lint errors coming up in this PR |
andrewseguin
removed
action: merge
Adds a custom tslint rule that ensures that we've added all of the necessary external modules to the Rollup config. This is helpful, because forgetting to add a module will log a warning, but it won't break the build necessarily, which is why we occasionally need PRs like angular#5930.
crisbeto
force-pushed
the
rollup-globals-rule
branch
from
5c046de to
321ee8b
Compare
crisbeto
added
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
6 participants
Adds a custom tslint rule that ensures that we've added all of the necessary external modules to the Rollup config. This is helpful, because forgetting to add a module will log a warning, but it won't break the build necessarily, which is why we occasionally need PRs like #5930.