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

WIP: switch to named UMD rxjs bundle #27220

Closed
wants to merge 1 commit into from

Conversation

gregmagolan
Copy link
Contributor

Testing new rxjs named UMD bundle that will be in next rxjs release

@mary-poppins
Copy link

You can preview 6f6f0b6 at https://pr27220-6f6f0b6.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 8537b54 at https://pr27220-8537b54.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 6027a87 at https://pr27220-6027a87.ngbuilds.io/.

@gregmagolan gregmagolan force-pushed the rxjs-named-umd-dist branch 2 times, most recently from 868f435 to aaf9716 Compare November 21, 2018 23:40
@mary-poppins
Copy link

You can preview 868f435 at https://pr27220-868f435.ngbuilds.io/.

@mary-poppins
Copy link

You can preview aaf9716 at https://pr27220-aaf9716.ngbuilds.io/.

@mary-poppins
Copy link

You can preview d3a017e at https://pr27220-d3a017e.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 4e26676 at https://pr27220-4e26676.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 57eb4f1 at https://pr27220-57eb4f1.ngbuilds.io/.

@mary-poppins
Copy link

You can preview d2ebdd7 at https://pr27220-d2ebdd7.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 2e3af73 at https://pr27220-2e3af73.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 9c5076f at https://pr27220-9c5076f.ngbuilds.io/.

@mary-poppins
Copy link

You can preview fc7da21 at https://pr27220-fc7da21.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 896d479 at https://pr27220-896d479.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 480773f at https://pr27220-480773f.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 028840d at https://pr27220-028840d.ngbuilds.io/.

@mary-poppins
Copy link

You can preview c302617 at https://pr27220-c302617.ngbuilds.io/.

@gregmagolan
Copy link
Contributor Author

Verifying that the angular build and the integration/bazel build will work with rxjs not being build from source with Bazel. Using a custom dist commit for rxjs that is based on ReactiveX/rxjs#4309.

//cc @alexeagle @alan-agius4

@gregmagolan
Copy link
Contributor Author

gregmagolan commented Nov 22, 2018

Note: integration/bazel prodserver is failing in the browser with an issue with extendStatics not being a function in the rxjs code that is bundled in. Not sure what this is about but may be some incompatibility with the latest rxjs typescript output and angular:

var __extends = (this && this.__extends) || (function () {
    var extendStatics = function (d, b) {
        extendStatics = Object.setPrototypeOf ||
            ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
            function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
        return extendStatics(d, b);
    }
    return function (d, b) {
        >>>> extendStatics(d, b); <<<<
        function __() { this.constructor = d; }
        d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
    };
})();

@gregmagolan
Copy link
Contributor Author

gregmagolan commented Nov 22, 2018

Other issues are that the rxjs UMD bundle will now be named rxjs with ReactiveX/rxjs#4309 but there are no bundles for the secondary entry points such as rxjs/operators, rxjs/testing, etc... I added rxjs shims files to work-around these issues for running tests. Not sure what the best approach is. Perhaps add these secondary entry point shims to the rxjs.umd.js bundle itself in the rxjs build? Is it acceptable for a bundle to define multiple UMD modules?

^ @benlesh @mgechev @alexeagle @alan-agius4 @robwormald

@gregmagolan
Copy link
Contributor Author

A number of golden files have additional symbols with this change as well

@alan-agius4
Copy link
Contributor

alan-agius4 commented Nov 22, 2018

@gregmagolan, I think that each entry point should have its umd bundle with its own naming (ie. rollup each entrypoint separately)

That said, this will be a breaking change since to use the rxjs/operators you would need to include rxjs.operators.bundle.js

As an interm solution, what i’d suggest, is all the above but the. Merge everything into the main rxjs umd bundle.

I can take it, if needs be 😊

@mary-poppins
Copy link

You can preview e5a60f3 at https://pr27220-e5a60f3.ngbuilds.io/.

@gregmagolan
Copy link
Contributor Author

gregmagolan commented Nov 22, 2018

A little more data on the extendStatics issue with prodserver in integration/bazel:

rollup.umd.js has __extends defined like this:

    var extendStatics = Object.setPrototypeOf ||
        ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
        function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };

    function __extends(d, b) {
        extendStatics(d, b);
        function __() { this.constructor = d; }
        d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
    }

but individual .js files which are the bundled up for the production bundle in integration/bazel have it defined like this:

var __extends = (this && this.__extends) || (function () {
    var extendStatics = function (d, b) {
        extendStatics = Object.setPrototypeOf ||
            ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
            function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
        return extendStatics(d, b);
    }
    return function (d, b) {
        >>>>>>> extendStatics(d, b); <<<<<<<<
        function __() { this.constructor = d; }
        d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
    };
})();

the browser runtime error happens in the latter case where indicated

@mary-poppins
Copy link

You can preview b834b6f at https://pr27220-b834b6f.ngbuilds.io/.

@gregmagolan
Copy link
Contributor Author

The error being:

Uncaught TypeError: extendStatics is not a function
    at __extends (bundle.js:2748)
    at bundle.js:1966
    at bundle.js:1993
    at bundle.js:31184

@gregmagolan
Copy link
Contributor Author

Ahhhh. Looks like integration/bazel bundle.es6.js works fine so its not a rollup issue. Something breaks when downleveling to bundle.js.

@mary-poppins
Copy link

You can preview 6b88e02 at https://pr27220-6b88e02.ngbuilds.io/.

@gregmagolan
Copy link
Contributor Author

Ok. Finally tracked it down.

In the es6 bundle,

    var extendStatics = function (d, b) {
        extendStatics = Object.setPrototypeOf ||
            ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
            function (d, b) { for (var p in b)
                if (b.hasOwnProperty(p))
                    d[p] = b[p]; };
        return extendStatics(d, b);
    };
    function __extends(d, b) {
        extendStatics(d, b);
        function __() { this.constructor = d; }
        d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
    }

ends up before any uses of __extends.

After downleveling to es5, the line class ComponentFactoryBoundToModule extends ComponentFactory { is downleved to

    var ComponentFactoryBoundToModule = /** @class */ (function (_super) {
        __extends(ComponentFactoryBoundToModule, _super);

and you now have one use of __extends before its definition which causes the runtime error.

@gregmagolan
Copy link
Contributor Author

gregmagolan commented Nov 22, 2018

It seems like the underlying issue is that the rollup node resolve plugin being used rollup-plugin-node-resolve does not support the es2015 entry point so what is bundled is the module entry point which is the esm5 rxjs code which contains tslib.__extends which injects

var extendStatics = function (d, b) {
        extendStatics = Object.setPrototypeOf ||
            ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
            function (d, b) { for (var p in b)
                if (b.hasOwnProperty(p))
                    d[p] = b[p]; };
        return extendStatics(d, b);
    };
    function __extends(d, b) {
        extendStatics(d, b);
        function __() { this.constructor = d; }
        d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
    }

at that point.

There is a https://www.npmjs.com/package/rollup-plugin-node-resolve-next that uses the es2015 entry point if it is available (which it is in rxjs) and falls back to module and main but this plugin does not support the customResolveOptions: {moduleDirectory: nodeModulesRoot}} which we need in Bazel.

There is also a https://github.com/OasisDigital/rollup-plugin-node-resolve-angular which supports the 'es2015' field which I will look at.

@gregmagolan
Copy link
Contributor Author

gregmagolan commented Nov 23, 2018

Fixed the es2015 issue with rxjs. rollup_bundle will need an update here: bazelbuild/rules_nodejs#429 that depends on a change in rollup-plugin-node-resolve https://github.com/gregmagolan/rollup-plugin-node-resolve/commit/7a33dc249b69497aff3380a37a359158167b4801 which I'll make a PR for. With this change rollup_bundle will use the es2015 entry point of an npm module with fallback to module and then main.

@mary-poppins
Copy link

You can preview d37b9b1 at https://pr27220-d37b9b1.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 2e7bfe9 at https://pr27220-2e7bfe9.ngbuilds.io/.

@gregmagolan
Copy link
Contributor Author

gregmagolan commented Dec 5, 2018

Done testing. Not meant for merging.

rxjs named-UMD-bundle (ReactiveX/rxjs#4309) will allow us to not build rxjs from source with Bazel.

angular/angular build will be updated after the next release of rxjs and the changes will depend on bazelbuild/rules_nodejs#429 which in turn depends on rollup/rollup-plugin-node-resolve#186 / rollup/rollup-plugin-node-resolve#182 and a rollup-plugin-node-resolve release.

^ @alexeagle @IgorMinar @alan-agius4

@gregmagolan gregmagolan closed this Dec 5, 2018
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants