Make rxjs a side-effect free package (for webpack v4)

[IgorMinar]

Webpack is going to introduce a new feature called side-effect free module optimization in v4 (currently in alpha).

This optimization requires packages to opt-in to it and promise to contain only side-effect free ES modules by adding side-effects: false flag into the package.json. More info: https://github.com/webpack/webpack/tree/next/examples/side-effects

This is mostly true for rxjs except for the special modules that monkey patch the prototype (specifically rxjs, rxjs/add/operator/*, rxjs/add/observable/*). These modules are being deprecated anyway in v6 in favor of lettable operators and creation functions (observable factories), so we should just move that code into a separate package with reexports from the rxjs package to avoid breaking the world.

The redirects could look something like this:

rxjs/add/operator/foo in the rxjs package (marked as side-effect free)

   import from 'rxjs-deprecations/add/operator/foo';

rxjs-deprecations/add/operator/foo in the rxjs-deprecations package (not marked as side-effect free)

import {Observable} from 'rxjs'

Obeservable.prototype.foo = ....

This way webpack does it's magic for people that use the new module ids (see #3145) and for people that still require old paths due to legacy code, all they have to do is to install rxjs-deprecations into their app (we could also consider making it a dependency of rxjs and auto installing it but that could cause issues and would promote the bad side-effecty behavior which is dangerous, so for that reason alone I'd prefer the legacy package to be an opt-in)

Note: this issue came out of discussion in angular/angular-cli#9069 (comment)

[IgorMinar]

btw the credit for the extra package and redirect idea goes to @jasonaden. I think it's a pretty rad idea.

[benlesh]

Would this effect the Symbol.observable ponyfill we have in RxJS? It technically has a side-effect if it polyfills.

[IgorMinar]

@benlesh can you point me to the code and explain how/when the polyfill does it's thing? I think we should be able to make it work, but it's good that you pointed it out.

[kwonoj]

@IgorMinar I believe this is it : https://github.com/ReactiveX/rxjs/blob/master/src/symbol/observable.ts

[IgorMinar]

thanks! in that case this is the only line with a side-effect:

rxjs/src/symbol/observable.ts

Line 12 in e308ff9

Symbol.observable = $$observable;

We have two options there:

1. Lie and be careful not to get caught

As rxjs internals always access the observable symbol via this exported const:

rxjs/src/symbol/observable.ts

Line 21 in e308ff9

export const observable = getSymbolObservable(root);

We should be fine. But I don't understand why we polyfill this at all in this way, so maybe we should consider the next option:

2. Stop polyfilling

If there is no strong reason to polyfill stuff in this way, then we should not do it or find a different way to do it. I don't understand why this is required so it's hard for me to suggest an alternative.

[kwonoj]

I recall it was to allow interop with TC-39 observable spec.

[IgorMinar]

If anyone wants interoperability in a browser that doesn't have the symbol they should load a polyfill. That's how it usually works. Why is rxjs different?

…

On Wed, Jan 10, 2018, 7:08 PM OJ Kwon ***@***.***> wrote: I recall it was to allow interop with TC-39 observable spec. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3212 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANM6HqYYxv3GsDhJD4wLeOs7dC5GCb3ks5tJXtEgaJpZM4RTjEA> .

[benlesh]

I can think of a few ways to get around polyfilling this and have everything still work. 👍

[simonbuchan]

As I mentioned in #3227, webpack 4.0.1 just added globbing or mapping support for sideEffects: webpack/webpack#6536
So you could walk back to having some side-effecting code if you wanted (but it probably doesn't make sense nowadays)

[emilio-martinez]

Moving in a direction that isn't tied to a specific bundler is probably the better way forward. Webpack's great, but not everyone can or might want to use Webpack.

[simonbuchan]

Well sideEffects is a webpack 4 thing in the first place? In the worst case of someone using another bundler, they just have the exact same problem as they currently have.

[IgorMinar]

Yup. This change won't hurt anyone not using webpack. All the changes we are making take into consideration rollup, webpack and closure - three of the most popular bundling options

…

On Tue, Feb 27, 2018, 11:01 PM Simon Buchan ***@***.***> wrote: Well sideEffects is a webpack 4 thing in the first place? In the worst case of someone using another bundler, they just have the exact same problem as they currently have. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3212 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANM6PNkpNcyGwVhPY6jesMVmiDjHnNgks5tZPnlgaJpZM4RTjEA> .

[felixfbecker]

So if a package imports a non-side-effect-free package, can it still be side-effect-free?
My gut would tell me know, because what packages a package uses/imports internally is an implementation detail, and all I as a consumer care about is "if I import your package, could it cause a side effect?" (to which the answer seems to be yes).

Also voting for a precise package name like rxjs-add over rxjs-deprecations -- the latter sounds like a bag that's gonna get grow and grow over time and I don't see a benefit to bundling all deprecated functionality all in one package.

[simonbuchan]

So if a package imports a non-side-effect-free package, can it still be side-effect-free?

Yep! It's actually modules that a side-effect-free or not - hence the globbing in the PR I mentioned above. If I understand it correctly, it essentially means "the module init code has side effects other than creating export declarations, so it is not safe to re-write imports of this module's re-exports to their original exporting module, since it would change behavior" - that is, module-level tree-shaking without requiring expensive code analysis.

[benlesh]

done.

[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.