fix(bazel): disable treeshaking when generating FESM and UMD bundles #32069
Conversation
alan-agius4
added
target: patch
alan-agius4
added
effort1: hours
action: review
There has been a regression where enabling rollup treeshaking causes errors during runtime because it will drop const access which will always evaluate to true or false. However, such `const` in `@angular/core` cannot be dropped because their value is changed when NGCC is run on `@angular/core`
VE
```
const SWITCH_IVY_ENABLED__POST_R3__ = true;
const SWITCH_IVY_ENABLED__PRE_R3__ = false;
const ivyEnabled = SWITCH_IVY_ENABLED__PRE_R3__;
```
Ivy (After NGCC)
```
const SWITCH_IVY_ENABLED__POST_R3__ = true;
const SWITCH_IVY_ENABLED__PRE_R3__ = false;
const ivyEnabled = SWITCH_IVY_ENABLED__POST_R3__;
```
FESM2015
```
load(path) {
/** @type {?} */
const legacyOfflineMode = this._compiler instanceof Compiler;
return legacyOfflineMode ? this.loadFactory(path) : this.loadAndCompile(path);
}
```
ESM2015
```
load(path) {
/** @type {?} */
const legacyOfflineMode = !ivyEnabled && this._compiler instanceof Compiler;
return legacyOfflineMode ? this.loadFactory(path) : this.loadAndCompile(path);
}
```
From the above we can see that `ivyEnabled ` is being treeshaken away when generating the FESM bundle which is causing runtime errors such as `Cannot find module './lazy/lazy.module.ngfactory'` since in Ivy we will always load the factories.
| @@ -111,6 +111,41 @@ Hello | |||
| See the Apache Version 2.0 License for specific language governing permissions | |||
| and limitations under the License. | |||
| ***************************************************************************** */ | |||
| /* global Reflect, Promise */ | |||
There was a problem hiding this comment.
Note: with this approach, all methods from tslib will be inlined.
I know that @alexeagle had in mind to put tslib as a peerDepedency instead of it being a direct dependency, which eventually would make tslib helpers as externals for UMD bundles as well.
There was a problem hiding this comment.
we could tell rollup that tslib is external right? I think that's the mechanism that would prevent it being bundled in; then we'd need to fix any package.json files to warn users using the peerDep mechanism. Do you plan to do that as a follow-up?
There was a problem hiding this comment.
Yes we could for umd, we are explicitly stating the we should include tslib
I would like to follow this up and take on TOOL-836, though would like to confirm that @IgorMinar is onboard, if my memory serves me well, he had some concerns not to include tslib as a direct dependency in version 6.
There was a problem hiding this comment.
I just came across this. tslib is now a peer dependency for all Angular packages. Though we still bundle tslib into the UMD bundles. This seems to cause unnecessary bloat when multiple UMD bundles are loaded.
Ideally we'd just not bundle tslib into it, and require consumers to load tslib before. I think we don't do this right now because there is no UMD bundle for tslib.
There was a problem hiding this comment.
It might be worth looking into this again. Potentially we could help tslib with providing an UMD version so that we can move forward with this.
There was a problem hiding this comment.
The problem with not inlining tslib is that it be a breaking change because we will breaks users who uses umds.
See also: #32167 (review)
What is the problem you are seeing? Or is it just a size matter?
There was a problem hiding this comment.
It's just a matter of size. It makes sense that we continue bundling it for backwards compatibility. Though, I'm wondering if we could/should revisit this in the future. It seems not ideal to include tslib into the bundles multiple times, given that rxjs is external too.
There was a problem hiding this comment.
Additionally the fact that tree-shaking had to be disabled here, doesn't make it better. As you already stated, it means that the whole tslib library is included even if only parts are needed.
|
Note: this regressions is causing Ivy E2E tests in the CLI to be break. |
|
Was going to file the issue but fortunately found this PR. I had to roll back to 8.2.0 due to this issue since |
|
And, well, it's not Bazel-only specific since this code stopped working with direct @ngtools/webpack.AngularCompilerPlugin use for compiling (because |
|
@vladimiry, this bazel rule is to publish the Angular framework. The bug itself lies in the shipped |
|
Thanks for the clarification, then there should be another |
|
Not quite, since this is a bundling issue, there are no code changes required in |
|
Do I get it right that 8.2.1 published on npm repository is going to be marked as |
|
I think the preferred approach would be to cut another release versioned 8.2.2 |
|
My point is that deprecating 8.2.1 could reduce potential damage. Anyway looking forward to trying 8.2.2. |
|
FYI - @gregmagolan |
There was a problem hiding this comment.
LGTM. Unfortunate that all tree-shaking has to be disabled but doesn't look like rollup has fine grained options other than
--no-treeshake Disable tree-shaking optimisations
--no-treeshake.annotations Ignore pure call annotations
--no-treeshake.propertyReadSideEffects Ignore property access side-effects
|
@gregmagolan, yeah there doesn't seem to be an option to disable such treeshaking. |
alan-agius4
added
action: merge
…32069) There has been a regression where enabling rollup treeshaking causes errors during runtime because it will drop const access which will always evaluate to true or false. However, such `const` in `@angular/core` cannot be dropped because their value is changed when NGCC is run on `@angular/core` VE ``` const SWITCH_IVY_ENABLED__POST_R3__ = true; const SWITCH_IVY_ENABLED__PRE_R3__ = false; const ivyEnabled = SWITCH_IVY_ENABLED__PRE_R3__; ``` Ivy (After NGCC) ``` const SWITCH_IVY_ENABLED__POST_R3__ = true; const SWITCH_IVY_ENABLED__PRE_R3__ = false; const ivyEnabled = SWITCH_IVY_ENABLED__POST_R3__; ``` FESM2015 ``` load(path) { /** @type {?} */ const legacyOfflineMode = this._compiler instanceof Compiler; return legacyOfflineMode ? this.loadFactory(path) : this.loadAndCompile(path); } ``` ESM2015 ``` load(path) { /** @type {?} */ const legacyOfflineMode = !ivyEnabled && this._compiler instanceof Compiler; return legacyOfflineMode ? this.loadFactory(path) : this.loadAndCompile(path); } ``` From the above we can see that `ivyEnabled ` is being treeshaken away when generating the FESM bundle which is causing runtime errors such as `Cannot find module './lazy/lazy.module.ngfactory'` since in Ivy we will always load the factories. PR Close #32069
gkalpak
added
the
action: cleanup
kara
removed
the
action: cleanup
|
@alan-agius4 Since the review missed the merge by ~1 min, it was merged without the typo fix. Would you mind addressing the feedback in a follow-up? #32069 (comment) |
|
@kara sure! |
…ngular#32069) There has been a regression where enabling rollup treeshaking causes errors during runtime because it will drop const access which will always evaluate to true or false. However, such `const` in `@angular/core` cannot be dropped because their value is changed when NGCC is run on `@angular/core` VE ``` const SWITCH_IVY_ENABLED__POST_R3__ = true; const SWITCH_IVY_ENABLED__PRE_R3__ = false; const ivyEnabled = SWITCH_IVY_ENABLED__PRE_R3__; ``` Ivy (After NGCC) ``` const SWITCH_IVY_ENABLED__POST_R3__ = true; const SWITCH_IVY_ENABLED__PRE_R3__ = false; const ivyEnabled = SWITCH_IVY_ENABLED__POST_R3__; ``` FESM2015 ``` load(path) { /** @type {?} */ const legacyOfflineMode = this._compiler instanceof Compiler; return legacyOfflineMode ? this.loadFactory(path) : this.loadAndCompile(path); } ``` ESM2015 ``` load(path) { /** @type {?} */ const legacyOfflineMode = !ivyEnabled && this._compiler instanceof Compiler; return legacyOfflineMode ? this.loadFactory(path) : this.loadAndCompile(path); } ``` From the above we can see that `ivyEnabled ` is being treeshaken away when generating the FESM bundle which is causing runtime errors such as `Cannot find module './lazy/lazy.module.ngfactory'` since in Ivy we will always load the factories. PR Close angular#32069
…ngular#32069) There has been a regression where enabling rollup treeshaking causes errors during runtime because it will drop const access which will always evaluate to true or false. However, such `const` in `@angular/core` cannot be dropped because their value is changed when NGCC is run on `@angular/core` VE ``` const SWITCH_IVY_ENABLED__POST_R3__ = true; const SWITCH_IVY_ENABLED__PRE_R3__ = false; const ivyEnabled = SWITCH_IVY_ENABLED__PRE_R3__; ``` Ivy (After NGCC) ``` const SWITCH_IVY_ENABLED__POST_R3__ = true; const SWITCH_IVY_ENABLED__PRE_R3__ = false; const ivyEnabled = SWITCH_IVY_ENABLED__POST_R3__; ``` FESM2015 ``` load(path) { /** @type {?} */ const legacyOfflineMode = this._compiler instanceof Compiler; return legacyOfflineMode ? this.loadFactory(path) : this.loadAndCompile(path); } ``` ESM2015 ``` load(path) { /** @type {?} */ const legacyOfflineMode = !ivyEnabled && this._compiler instanceof Compiler; return legacyOfflineMode ? this.loadFactory(path) : this.loadAndCompile(path); } ``` From the above we can see that `ivyEnabled ` is being treeshaken away when generating the FESM bundle which is causing runtime errors such as `Cannot find module './lazy/lazy.module.ngfactory'` since in Ivy we will always load the factories. PR Close angular#32069
|
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. |
There has been a regression where enabling rollup treeshaking causes errors during runtime because it will drop const access which will always evaluate to true or false. However, such
constin@angular/corecannot be dropped because their value is changed when NGCC is run on@angular/coreVE
Ivy (After NGCC)
FESM2015
ESM2015
From the above we can see that
ivyEnabledis being treeshaken away when generating the FESM bundle which is causing runtime errors such asCannot find module './lazy/lazy.module.ngfactory'since in Ivy we will always load the factories.