Skip to content
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(bazel): disable treeshaking when generating FESM and UMD bundles #32069

Closed
wants to merge 2 commits into from

Conversation

@alan-agius4
Copy link
Contributor

commented Aug 9, 2019

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.

@alan-agius4 alan-agius4 requested review from angular/fw-dev-infra as code owners Aug 9, 2019

@googlebot googlebot added the cla: yes label Aug 9, 2019

@alan-agius4 alan-agius4 force-pushed the alan-agius4:rollup-tree-shaking branch from a69954b to ce777ce Aug 9, 2019

@ngbot ngbot bot added this to the needsTriage milestone Aug 9, 2019

@alan-agius4 alan-agius4 force-pushed the alan-agius4:rollup-tree-shaking branch from ce777ce to 092696d Aug 9, 2019

fix(bazel): disable treeshaking when generating FESM and UMD bundles
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.

@alan-agius4 alan-agius4 force-pushed the alan-agius4:rollup-tree-shaking branch from 092696d to 78205a6 Aug 9, 2019

@@ -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 */

This comment has been minimized.

Copy link
@alan-agius4

alan-agius4 Aug 9, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@alexeagle

alexeagle Aug 9, 2019

Contributor

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?

This comment has been minimized.

Copy link
@alan-agius4

alan-agius4 Aug 9, 2019

Author Contributor

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.

@alan-agius4

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Note: this regressions is causing Ivy E2E tests in the CLI to be break.

@vladimiry

This comment has been minimized.

Copy link

commented Aug 9, 2019

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 NgModuleFactoryLoader.load got broken in ivy mode.

@vladimiry

This comment has been minimized.

Copy link

commented Aug 9, 2019

And, well, it's not Bazel-only specific since this code stopped working with direct @ngtools/webpack.AngularCompilerPlugin use for compiling (because legacyOfflineMode always resolves to true with 8.2.1).

@alan-agius4

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@vladimiry, this bazel rule is to publish the Angular framework.

The bug itself lies in the shipped @angular/core package as certain code which is required at runtime was removed..

@vladimiry

This comment has been minimized.

Copy link

commented Aug 9, 2019

Thanks for the clarification, then there should be another @angular/core-related issue/PR I've not located yet. I got to this PR using legacyOfflineMode as a search keyword since I debugged the stuff to this stage.

@alan-agius4

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

Not quite, since this is a bundling issue, there are no code changes required in @angular/core.

@vladimiry

This comment has been minimized.

Copy link

commented Aug 9, 2019

Do I get it right that 8.2.1 published on npm repository is going to be marked as deprecated to reduce probability of 8.2.1 use by the developers? Since there is no ivyEnabled flag in published pre-compiled fesm* folders and there is no way to get it back for the already published version.

@alan-agius4

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

I think the preferred approach would be to cut another release versioned 8.2.2

@vladimiry

This comment has been minimized.

Copy link

commented Aug 9, 2019

My point is that deprecating 8.2.1 could reduce potential damage. Anyway looking forward to trying 8.2.2.

vladimiry added a commit to vladimiry/ElectronMail that referenced this pull request Aug 9, 2019

@vikerman

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

FYI - @gregmagolan

@gregmagolan gregmagolan self-requested a review Aug 9, 2019

@gregmagolan
Copy link
Contributor

left a 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
@alan-agius4

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@gregmagolan, yeah there doesn't seem to be an option to disable such treeshaking.

@josephperrott
Copy link
Contributor

left a comment

LGTM for devinfra

kara added a commit that referenced this pull request Aug 9, 2019

fix(bazel): disable treeshaking when generating FESM and UMD bundles (#…
…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

kara added a commit that referenced this pull request Aug 9, 2019

@kara kara removed the PR action: cleanup label Aug 9, 2019

pull bot pushed a commit to Potapy4/angular that referenced this pull request Aug 9, 2019

@kara kara closed this in 4f37487 Aug 9, 2019

@kara

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

@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)

@alan-agius4 alan-agius4 deleted the alan-agius4:rollup-tree-shaking branch Aug 10, 2019

@alan-agius4

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

@kara sure!

pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this pull request Aug 12, 2019

fix(bazel): disable treeshaking when generating FESM and UMD bundles (a…
…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

pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this pull request Aug 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.