feat(@angular-devkit/build-angular): show warnings when depending on CommonJS#17250
Conversation
alan-agius4
added
PR state: blocked
target: major
|
Blocking until 9.1 branch is created. |
alan-agius4
added
the
needs: discussion
|
To discuss:
|
|
Decided that we should go with an builder option that allows a list of packages to suppress the warning for. |
|
|
||
| it('should not show warning when depending on a Common JS bundle which is allowed', async () => { | ||
| const overrides = { | ||
| allowedCommonJsDepedencies: [ |
There was a problem hiding this comment.
@filipesilva / @clydin should allowing a CommonJS dep allow also it's CommonJS transitive dependency automatically?
There was a problem hiding this comment.
Hmm that's a tricky one. Transitive dependencies in general really.
Realistically speaking many project, especially larger ones, will have dozens of transitive commonjs dependencies. So they get these changes and see a screenful of names they have to allow. That's not great.
On the other hand the whole point of these warnings is to let users know where in their dep graph are there deoptimizations. So it's not any help to not show the transitives.
To make informed decisions the user will also need the full dependency path to the commonjs dependency.
Which then leads to your specific question: should we just show deps up to a commonjs dep, then ignore that subgraph? Yeah that seems a good way to cut down on the total shown.
alan-agius4
removed
the
needs: discussion
filipesilva
left a comment
filipesilva
left a comment
There was a problem hiding this comment.
LGTM, just needs that typo fixed.
…CommonJS.
Depending on CommonJS modules is know to cause optimization bailouts. With this change when running a browser build and scripts optimization is enabled we display a warning.
To suppress the warning for a particular package, users can use the `allowedCommonJsDepedencies` builder options.
Example:
```
"build": {
"builder": "@angular-devkit/build-angular:browser",
"options": {
...
"allowedCommonJsDepedencies": ["bootstrap"]
},
}
```
Reference: TOOL-1328
alan-agius4
added
action: merge
|
I see that you just added the If you want your PR to be merged, it has to pass all the CI checks. If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help. |
|
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. |
4 participants
feat(@angular-devkit/build-angular): show warnings when depending on CommonJS.
Depending on CommonJS modules is know to cause optimization bailouts. With this change when running a browser build and scripts optimization is enabled we display a warning.
To suppress the warning for a particular package, users can use the
allowedCommonJsDepedenciesbuilder options.Example:
Reference: TOOL-1328