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

feat(Symbol.observable): is no longer polyfilled #3387

Merged

Conversation

@benlesh
Copy link
Member

benlesh commented Mar 6, 2018

BREAKING CHANGE: RxJS will no longer be polyfilling Symbol.observable. That should be done by an actual polyfill library. This is to prevent duplication of code, and also to prevent having modules with side-effects in rxjs.

@benlesh benlesh requested a review from kwonoj Mar 6, 2018
@benlesh

This comment has been minimized.

Copy link
Member Author

benlesh commented Mar 7, 2018

@benlesh benlesh requested a review from jayphelps Mar 7, 2018
@rxjs-bot

This comment has been minimized.

Copy link

rxjs-bot commented Mar 7, 2018

Messages
📖

CJS: 1302.1KB, global: 692.3KB (gzipped: 113.3KB), min: 133.7KB (gzipped: 29.3KB)

Generated by 🚫 dangerJS

@benlesh benlesh force-pushed the benlesh:no-more-symbol-observable-polyfill branch from a725e97 to dc575ba Mar 7, 2018
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 7, 2018

Coverage Status

Coverage decreased (-0.004%) to 97.447% when pulling 433d774 on benlesh:no-more-symbol-observable-polyfill into 2507518 on ReactiveX:master.

really *required*, however, they should be warned to give them a
clue as to why they might be seeing errors when trying to convert
ObservableLikes to Observables */
if (!Symbol || !Symbol.observable) {

This comment has been minimized.

Copy link
@jayphelps

jayphelps Mar 7, 2018

Member

This will need to do typeof Symbol because otherwise it still will do Symbol is not defined. See #3394 for reference example

*/
export const $$observable = observable;
export const $$observable = Symbol && Symbol.observable || '@@observable';

This comment has been minimized.

Copy link
@jayphelps

jayphelps Mar 7, 2018

Member

Same as above, prolly easiest to wrap in a function like done in #3394 so only need to check once. Also will allow you to easily make sure the type is of symbol instead of any as it is in this case.

This comment has been minimized.

Copy link
@benlesh

benlesh Mar 7, 2018

Author Member

Wrapping it in a function will only make it easier to test. The module body itself should only be called once, no?

This comment has been minimized.

Copy link
@jayphelps

jayphelps Mar 7, 2018

Member

@benlesh Partially, though it also makes it easier to only need to do the typeof Symbol check once.

@jayphelps

This comment has been minimized.

Copy link
Member

jayphelps commented Mar 7, 2018

btw I think these symbols are no longer publicly exported except for in the UMD build. Which one is intended?

I'm not entirely sure if we should be exporting any of them. If we did, seems like Symbol.observable would be the only one we have any business doing so but even then if someone wants to use one of them without native support (no JS env has native support yet, so right now that means any time they want to use it), they should include a polyfill IMO and then rxjs will end up using it transparently anyway so no need to access variable RxJS creates to reference it.

I can see a possible argument about allowing libraries that use RxJS to add support for Symbol.observable without including the polyfill automatically (which would mutate Symbol, forcing it on their users). My gut says that in those cases they too should either follow our lead with "@@observable" string as a fallback, or just choose not to implement support for Symbol.observable when it's not defined. In fact, that's probably what we should be doing too? I'm not sure when the "@@observable" approach is useful?

@benlesh

This comment has been minimized.

Copy link
Member Author

benlesh commented Mar 7, 2018

I don't disagree, @jayphelps, I think we should only be implementing/executing some logic if Symbol.observable is defined... this was just the quickest incremental step.

BREAKING CHANGE: RxJS will no longer be polyfilling Symbol.observable. That should be done by an actual polyfill library. This is to prevent duplication of code, and also to prevent having modules with side-effects in rxjs.
@benlesh benlesh force-pushed the benlesh:no-more-symbol-observable-polyfill branch from 21c54e1 to 433d774 Mar 8, 2018
Copy link
Collaborator

IgorMinar left a comment

looks good now!

@benlesh benlesh dismissed jayphelps’s stale review Mar 8, 2018

Dismissed after offline discussion.

@benlesh benlesh merged commit 4a5aaaf into ReactiveX:master Mar 8, 2018
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.004%) to 97.447%
Details
@lock

This comment has been minimized.

Copy link

lock bot commented Jun 6, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.