chore(build): Use rollup to bundle cjs build

[zertosh]

This PR changes how the "Global" (aka umd) build is bundled and transpiled.

Before: TS-to-ES5 -> Browserify
After: TS-to-ES6 -> Rollup -> Babel-to-ES5

The new build is considerably smaller and loads much faster:

File	Size (before)	Size (after)	Load (before)	Load (after)
Rx.js	608K	514K	110ms	34ms
Rx.min.js	222K	168K	95ms	26ms
Rx.min.js.map	448K	338K	n/a	n/a

To test load time, I ran for i in {1..5}; do node -p "s=Date.now();require('path/to/bundle');Date.now()-s"; done.

A few notes:

• The improvements come from debuping the helper code (class creation, etc.), and from rollup's single body bundling.
• In order to dedupe the helper code, tranpiling has be done post-bundling, which is why I'm using babel. AFAIK, TS can't transpile ES6 to ES5 (or can it?).
• Babel's loose-mode seemed to more closely match TS's ES5 output.
• I tested the resulting bundle by using it nuclide.

[coveralls]

Coverage remained the same at 97.041% when pulling cb68c1d on zertosh:rollup into ece1b89 on ReactiveX:master.

[kwonoj]

While it brings additional steps of generating build, I'm 👍 since outcome looks good.

One question - does sourcemap for min version points original code correctly?

[zertosh]

One question - does sourcemap for min version points original code correctly?

@kwonoj The sourcemaps are generated by GCC and they're based off the post-transpiled/post-bundled output (you have to pass debug: true to browserify for it to preserve and output source maps). That didn't change, so the short answer is "they should be the same as before".

But this brings up another point, and it's that the source maps should be based on pre-bundled output (maybe also pre-transpiled output). This would make them quite large (prob a few MB), but maybe more useful.

[kwonoj]

This would make them quite large (prob a few MB), but maybe more useful.

(My personal 5c) I'd prefer sourcemap to be more accurate even it'll take more space. Prod. code can strip sourcemap anyway since it's not inlined.

[zertosh]

@kwonoj sure. Would you like me to make that change here or in another PR? (Since this PR keeps the original behavior)

[kwonoj]

Would you like me to make that change here or in another PR? (Since this PR keeps the original behavior)

I agree with that, but let's consider that after got agreement on this PR and merged. I'm 👍 , need additional approval to check in.

[Rich-Harris]

Hey – Rollup author here, I got alerted to this thread. The sourcemap thing is an issue I've had to deal with in a few different contexts, so I wrote a tool called sorcery to generate a sourcemap that accounts for an arbitrary number of sourcemap-generating transformations. It's not bulletproof (nothing involving sourcemaps is, in my experience!) but it might be useful so I thought I'd mention it.

It ought to be possible to dedupe helpers using rollup-plugin-babel, though YMMV (I don't use it personally, so I'm not 100% sure it's kept up with changes to Babel) – in theory it will generate slightly leaner output, and the build will be faster than transpiling the whole bundle.

[jayphelps]

LGTM, we're going to continue to discuss more accurate source map support.

[zertosh]

@Rich-Harris: I tried rollup-plugin-babel but it didn't actually dedupe the helpers :/ The best output I got was from transpiling after the bundling. As far as build speed impact, this adds a second or two - but that's only really run for publishing, doesn't really affect dev-time speed.

[zertosh]

Thanks @jayphelps and @kwonoj! Nuclide would really appreciate a 5.0.0-rc.2 release so we can start using this 😄

[jayphelps]

@zertosh side question, we've been trying to accumulate use cases from our Electron users who don't use a bundler. Since Nuclide is one of them, can you provide any insight? If a bundler is used, operator patching becomes the way to reduce the size of your shipped app.

We've heard of several who are worried about file size but don't use bundling to reduce this (and reduce require() file system pressure), but we don't have any real world experience with Electron to know if there are additional nuances that make bundling impractical, etc. If build times were the concern Electron is unique in the sense that you could get away with not bundling when doing local dev, but then bundle as part of a final packaging step.

[zertosh]

@jayphelps we don't bundle in Nuclide – for various reasons: we have several node binary modules, code in multiple languages (python and Java), and Nuclide is actually two apps the "client" and the "server" (we'd need two bundles - more dev overhead). All of these make bundling more complicated than what we're willing to deal with right now.

Our concern is not so much with size but load times - though there's some correlation between the two, that's not always the case. Since bundling is not option right now, the next best thing is to load the UMD bundle - which is considerably faster to load than the unbundled entry point. This is transparent to devs because at transpile time, we convert the source on imports from rxjs to rxjs/bundles/Rx.min.js.

Loading rxjs in Node is not cheap:

$ for i in {1..5}; do node -p "s=Date.now();require('rxjs');Date.now()-s"; done
178
187
185
184
191

I'd love it if the UMD bundle were brought down to the root of the package - so that it's easier to manually require in scripts. Maybe rename rxjs/Rx.js to rxjs/Rx.all.js (but still as the entry point), and rxjs/bundles/Rx.js to rxjs/rx.js (lowercase).

[kwonoj]

@jayphelps we don't bundle it as well due to similar reason to nuclide. In short, when it comes electron having various native-binary modules, it is not trivial effort to setup bundler (to exclude those modules, and setup other routing to load those modules as well) in addition to it requires 2 set of bundles eventually, one for main process and other for browser renderer process.

What we do in our codebase for Rx is straightforward - instead of import everything, import patch operator individually for necessary ones. (import '.../add/../xxx')

[zertosh]

Oh, and we don't patch import operators because we don't have a way to enforce via flow that you didn't forget something.

[kwonoj]

@zertosh yup, we're able to do it since we're using TypeScript in our codebase.

[zertosh]

Interesting... @kwonoj can you show me what that looks like?

[kwonoj]

@zertosh idea's simple - as latest typescript compiler (or language service) do understands module argumentation, if you write any consumer code like

import {Observable} from 'rxjs/Observable';
import 'rxjs/add/observable/of';

Observable.of //type inference will work since language service accepts module augmentation

(I just wrote down code so it might not work via ctrl-v, but I assume you get idea)

[trxcllnt]

@zertosh @kwonoj @jayphelps it seems after this PR the files in dist/global aren't in UMD wrappers any more. Am I missing something, is this expected, or is this a regression?

[jayphelps]

@trxcllnt I'm seeing it here: https://unpkg.com/@reactivex/rxjs@5.0.0-rc.1/dist/global/Rx.js

Also one of my demo jsbins still works: http://jsbin.com/jexomi/edit?html,js,output

Can you clarify?

(function (f) {
  if(typeof exports === "object" && typeof module !== "undefined") {
    module.exports = f()
  } else if(typeof define === "function" && define.amd) {
    define([], f)
  } else {
    var g;
    if(typeof window !== "undefined") {
      g = window
    } else if(typeof global !== "undefined") {
      g = global
    } else if(typeof self !== "undefined") {
      g = self
    } else {
      g = this
    }
    g.Rx = f()
  }
})

[trxcllnt]

@jayphelps was this PR included in the rc.1 build? I'm building the project locally and don't have the UMD wrapper.

[jayphelps]

@trxcllnt nope! good catch!! I'm seeing the same thing locally. I'll check it out locally tomorrow (unless of course you find it). can you create a ticket to track?

related: I've been planning to bring up at our next meeting the idea of publishing canary builds of master to npm. The community could then run their CI against it to keep an eye out for breaking changes and also bug us if we break something unknowingly before we cut a real release.

[jayphelps]

@zertosh any initial thoughts? you mentioned you had used it in your local builds without issue so any change we make to fix this for others may break however it worked in yours nevermind, it should work the same for you cause it'll detect it's in a CJS env.

[Brooooooklyn]

I think TS => ES5 code + ES 6 module is a better way to bundle

[jayphelps]

@Brooooooklyn we're talking about UMD bundles, which cannot have ES modules in them (would be a syntax error)

were you maybe missing a step in there or am I misunderstanding?

[Brooooooklyn]

@jayphelps rollup is able to bundle ES6 module to UMD without babel, and if your code is compiled to ES5 by TypeScript using tsc entry.ts -m ES2015 --target ES5, you could do the TS => ES5 code + ES6 module=>use rollup => ES5 UMD code without babel.

I've do such things for a long time in our project https://github.com/teambition/teambition-sdk/blob/master/tools/tasks/bundle.ts#L32

😉

[zertosh]

@jayphelps I used it in node so CJS was enough. I'll add the UMD wrapper - sorry.

@Brooooooklyn If you do TS => ES5 code + ES6 module=>use rollup => ES5 UMD code you're going to have duplicate helpers.

[zertosh]

@jayphelps @Brooooooklyn #2091

[Brooooooklyn]

@zertosh you could use

tsc entry.ts -m ES2015 --target ES5 --noEmitHelpers

and

// entry.ts
import 'tslib'

to avoid duplicate helpers.

tslib is official external helper lib.

[Brooooooklyn]

My worry about this pull request is you introduce babel here, and there is already using TypeScript. We shouldn't use two kinds of transpiler through TypeScript could work fine here in this project, this would bring more risks.

[kwonoj]

I'd agree in general to have different transpilers in build chain is not ideal, it's tempting options to use TSXC entirely to generate global builds.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.