-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
How to include RxJS operators with Angular 2 #4543
Comments
Related... @vsavkin pointed out that because of how RxJS 5 creates it's default Observable Export, the Also Related: There has been discussion on the ReactiveX side of just including the core operators as part of Observable. This is because it would make certain optimizations more straight forward in terms of doing things like overriding some operators on ScalarObservable, for example. It might also slightly reduce the output size by eliminated some of the module-related output code. I have my doubts we'll do this as this point, but it's an option. |
Related discussion on RxJS side: ReactiveX/rxjs#464 |
In the short-term, I think it's probably best to just depend on the main RxJS 5 library and it's output, because people will need and want those operators. Long-term, I think we can probably come up with something more interesting |
this looks very promising https://github.com/rollup/rollup |
@robwormald ... yeah @jayphelps just mentioned that to me this morning. He named a couple of caveats (which now escape me) |
Biggest issues I see: angular/rxjs are written in TypeScript and it only supports ES6. You technically could transpile TS to ES6 then run it through rollup but WTF....It also requires consumers to use it as their module packer, which is tough in such a crowded field with people loving their grunt/gulp/broccoli/browserify/webpack/whatever. So..angular would have to use it. 😟 or if Rx isn't exposed to users of angular, internally it would have to be used. Instead of creating yet another module packer, the community needs to create a library that only just does AST in -> AST out and can be bolted onto any module packer (some with more work than others, depending on whether they use ASTs or not and which format). I'm probably missing something because there's probably a good reason @Rich-Harris didn't go this route? |
@jayphelps I'd be interested to better understand your needs – is the issue that the bundle needs to be exported using a custom format?
Two main reasons – performance and code readability. Generating code from an AST is more costly than just manipulating the original source code (rollup uses a utility called magic-string to do so in a sourcemap-aware way), and you don't have to jump through any hoops to preserve original formatting/comments/etc |
@Rich-Harris not sure what you mean by "issue". Theres the underlying quest to ship a monolithic project like RxJS and have an easy way for consumers to shake off unused operators. In considering rollup, I was merely pointing out that it isn't feasible in this angular case because it's TypeScript and if that's somehow worked around it would still require angular devs to actually use rollup as the module transpiler which I question the feasibility of and cringe at the yet-another-module-transpiler syndrome we go through every year it feels like. Since TS and ES share the exact same syntax of import/export, it seems it would possible to parse only these to build up a module tree and ignore everything else in the files. After the shaking, then the source is fed into the normal build pipelines, including their preferred module transpilation process because we could just remove files, leaving the kept ones untouched. Thoughts? Would certainly incur more perf penalty than the transpiler directly being aware, but is more flexible (devs can still make love to their preferred language TS/ES and transpiler) and devs could limit its usage to prod builds, when build time is relatively less of a concern. |
On second thought...It's certainly more complicated than just parsing import/exports. Primarily on the exports side, because if a module exports just identifier, if you don't parse the rest of the file you can't shake off the code the identifier references, unless none of the exports in the file are ever imported, only then could you just exclude the entire file. I'm also not sure you could reliably parse an export without also parsing the inner body of item exported; e.g. if you export a function, you can't find the function boundry/closing curly without parsing the function body.. hmmm |
Exactly – for tree-shaking to work, you need to work on the level of individual statements rather than modules. In my view, modularising things to the extent that one tool is responsible for generating the dependency graph and another is responsible for doing the bundling is the wrong direction to be going in – aside from the inefficiencies it would create (and the fact that you'd need to create yet another bundler!), from a usability standpoint we need tools that are more integrated, not less.
I mean, you have to use something 😀
Rollup is an alternative to Browserify and Webpack, but not to Grunt/Gulp/Broccoli which basically just coordinate the activity of underlying tools like Browserify/Webpack/Rollup. |
Closing in favor of #4390 |
Though don't let me stop the bundling discussion, and feel free to add thoughts on #4390 :) |
@Rich-Harris 😋 I was referring to bundlers made for grunt/gulp specifically like gulp-es6-module-bundler. |
@jayphelps There must be a Rule 34 for Grunt and Gulp plugins! https://github.com/mcasimir/gulp-rollup and https://github.com/chrisprice/grunt-rollup already exist (gulp-es6-module-bundler is based on the long-since-deprecated es6-module-transpiler project) – there's no need to use it directly, it can slot into any existing build process |
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. |
I wanted to open an issue to track this, since we discussed this today and (I don't think) we came to any conclusions, and I'm unsure which issue this is being tracked.
The issue at hand is how to include RxJS operators as a dependency without greatly increasing the size of the library/total required download.
We can probably break solutions up into ideas for the short-term and ideas for the long-term.
cc/ @jeffbcross @vsavkin @IgorMinar @mhevery
The text was updated successfully, but these errors were encountered: