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

Update to nodejs rules 0.27 #29210

Closed

Conversation

gregmagolan
Copy link
Contributor

No description provided.

@gregmagolan gregmagolan requested review from a team as code owners March 10, 2019 19:38
@gregmagolan gregmagolan added the area: bazel Issues related to the published `@angular/bazel` build rules label Mar 10, 2019
@ngbot ngbot bot added this to the needsTriage milestone Mar 10, 2019
@gregmagolan gregmagolan added the target: patch This PR is targeted for the next patch release label Mar 10, 2019
@gregmagolan gregmagolan requested a review from a team as a code owner March 10, 2019 19:41
@alexeagle alexeagle changed the title Update to nodejs rules 0.27.3 Update to nodejs rules 0.27 Mar 11, 2019
@gregmagolan gregmagolan force-pushed the build/rules-nodejs-0.27.0 branch 6 times, most recently from 70ccdfc to 37b9d30 Compare March 11, 2019 17:48
@gregmagolan gregmagolan requested review from a team as code owners March 11, 2019 17:55
@gregmagolan
Copy link
Contributor Author

gregmagolan commented Mar 13, 2019

Note: material-unit-tests failure has numerous failures that look like this

Firefox 63.0.0 (Linux 0.0.0) MatPaginator with the default internationalization provider should re-render when the i18n labels change FAILED
    Error: BrowserDynamicTestingModule has no @NgModule annotation in node_modules/systemjs/dist/system.js line 4 > eval (line 1564)
    TestBedRender3</TestBedRender3.prototype._compileNgModule@node_modules/@angular/core/bundles/core-testing.umd.js:1564:23
    calcTransitiveScopesFor/<@node_modules/@angular/core/bundles/core-testing.umd.js:1597:139
    transitiveScopesFor/<@node_modules/@angular/core/bundles/core.umd.js:21168:17

if @__PURE__ annotations are stripped out of the angular bundles (which happens by default with the update to rollup 1.3.1.

The platform-browser-dynamic-testing.umd.js Ivy bundle contains a number of __PURE__ annotations including one for BrowserDynamicTestingModule:

    var BrowserDynamicTestingModule = /** @class */ (function () {
        function BrowserDynamicTestingModule() {
        }
        BrowserDynamicTestingModule.ngModuleDef = i0.ɵdefineNgModule({ type: BrowserDynamicTestingModule, exports: [testing$1.BrowserTestingModule] });
        BrowserDynamicTestingModule.ngInjectorDef = i0.defineInjector({ factory: function BrowserDynamicTestingModule_Factory(t) { return new (t || BrowserDynamicTestingModule)(); }, providers: [
                { provide: testing.TestComponentRenderer, useClass: DOMTestComponentRenderer },
            ], imports: [[testing$1.BrowserTestingModule]] });
        return BrowserDynamicTestingModule;
    }());
    /*@__PURE__*/ i0.ɵsetClassMetadata(BrowserDynamicTestingModule, [{
            type: i0.NgModule,
            args: [{
                    exports: [testing$1.BrowserTestingModule],
                    providers: [
                        { provide: testing.TestComponentRenderer, useClass: DOMTestComponentRenderer },
                    ]
                }]
        }], null, null);

There are also __PURE__ annotations in other Ivy bundles.

@gregmagolan
Copy link
Contributor Author

For reference, rollup version was updated here bazelbuild/rules_nodejs#584 which brough in the @__PURE__ annotation tree shaking feature.

packages/bazel/src/ng_package/ng_package.bzl Show resolved Hide resolved
# which tree shakes @__PURE__ annotations by default. We turn this feature off
# for ng_package as Angular bundles contain these annotations and there are
# test failures if they are removed. See comments in
# https://github.com/angular/angular/pull/29210 for more information.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah seems like a bug for these annotations to be tree-shaken in the testing bundles? since many tests need them in JIT mode?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a rollup bug. I think libraries need to be run rollup with this option turned off, and applications need to run with it turned on.

"rollup": "^0.41.6",
"rollup-plugin-commonjs": "^8.0.2",
"rollup-plugin-node-resolve": "2.0.0",
"rollup": "^1.1.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rollup needed an upgrade to fix test failures in the upgrade-phonecat-2-hybrid test. It seems like rollup 0.41.6 couldn't handle the angular bundles produced by rollup 1.3.1 (which is what ng_packager now uses since it shares the version with rollup_bundle).

@alexeagle alexeagle added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Mar 14, 2019
@alexeagle
Copy link
Contributor

Caretaker: has global approval

@matsko matsko added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release labels Mar 14, 2019
@matsko matsko closed this in 08231f0 Mar 14, 2019
matsko pushed a commit that referenced this pull request Mar 14, 2019
matsko added a commit that referenced this pull request Mar 15, 2019
kara added a commit to kara/angular that referenced this pull request Mar 15, 2019
matsko added a commit that referenced this pull request Mar 15, 2019
alexeagle added a commit to alexeagle/angular that referenced this pull request Mar 15, 2019
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: bazel Issues related to the published `@angular/bazel` build rules cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants