Fix symbols to work with Babel UMD exporting #2435

DzmitryShylovich · 2017-03-03T13:35:47Z

coveralls · 2017-03-03T13:43:16Z

Coverage increased (+0.0003%) to 97.688% when pulling 5c868fe on DzmitryShylovich:gh/2415 into 7c41c08 on ReactiveX:master.

benlesh

Give the observable symbol an alias where it's being used. I think Symbol_observable because that's what it represents.

benlesh · 2017-03-15T20:29:55Z

src/Rx.ts

@@ -171,7 +171,7 @@ import { QueueScheduler } from './scheduler/QueueScheduler';
 import { AnimationFrameScheduler } from './scheduler/AnimationFrameScheduler';
 import { $$rxSubscriber as rxSubscriber } from './symbol/rxSubscriber';
 import { $$iterator as iterator } from './symbol/iterator';
-import { $$observable as observable } from './symbol/observable';
+import { observable } from './symbol/observable';


This looks okay, but here (and other places in the codebase where you'd done this in this PR), you should probably do import { observable as Symbol_observable } or something like that. observable is just too generic a name, especially for this library.

I can use $$observable as an alias so only import will change observable as $$observable

benlesh · 2017-03-15T20:31:03Z

@DzmitryShylovich apologies for taking my time to review this. I've had a busy couple of weeks

DzmitryShylovich · 2017-03-15T20:32:27Z

@benlesh ok, np, I understand. new workplace etc :)

should I change iterator and scheduler symbols in the similar fashion?

coveralls · 2017-03-15T21:05:12Z

Coverage increased (+0.0003%) to 97.688% when pulling da5a95d on DzmitryShylovich:gh/2415 into 69d051b on ReactiveX:master.

benlesh · 2017-03-15T22:45:26Z

should I change iterator and scheduler symbols in the similar fashion?

Sure.. We'll just need to rename this PR to something like "Fix symbols to work with Babel UMD exporting".

Also: If you can please change the commit messages to be like fix(symbol/observable): will be exported as \observable` to better support Babel UMD and others`. That way this will show up in our automated change logs.

I'm still not certain how we should announce this change, and (ideally) deprecate the $$observable-type exports by v7. Maybe @IgorMinar has a recommendation?

benlesh

Update the other symbol exports to do the same thing
Update commit messages to be prefixed with fix(symbol/observable): words here

benlesh · 2017-03-15T22:47:04Z

Thank you for your help on this @DzmitryShylovich

coveralls · 2017-03-15T23:09:18Z

Coverage increased (+0.001%) to 97.689% when pulling 62e70b6 on DzmitryShylovich:gh/2415 into e4e7a18 on ReactiveX:master.

DzmitryShylovich · 2017-03-15T23:23:48Z

@benlesh done. PTAL

coveralls · 2017-03-15T23:23:49Z

Coverage increased (+0.001%) to 97.689% when pulling bc852fa on DzmitryShylovich:gh/2415 into e4e7a18 on ReactiveX:master.

benlesh · 2017-03-16T19:05:26Z

src/Subject.ts

@@ -5,7 +5,7 @@ import { Subscriber } from './Subscriber';
 import { ISubscription, Subscription, TeardownLogic } from './Subscription';
 import { ObjectUnsubscribedError } from './util/ObjectUnsubscribedError';
 import { SubjectSubscription } from './SubjectSubscription';
-import { $$rxSubscriber } from './symbol/rxSubscriber';
+import { rxSubscriber as Symbol_rxSubscriber } from './symbol/rxSubscriber';


haha... GOTCHA! rxSubscriber doesn't exist on Symbol, really... (i.e. Symbol.rxSubscriber is not at thing)... so you can probably just leave this as rxSubscriber or as rxSubscriberSymbol or something)

Symbol.rxSubscriber is not at thing

https://github.com/ReactiveX/rxjs/blob/master/src/Rx.ts#L212 ?

http://plnkr.co/edit/SS0t7wCUh2ANZiVGeTNi?p=preview I can import it. or what do you mean by not a thing? :)

@DzmitryShylovich oh I meant that Symbol.observable -> Symbol_observable, Symbol.iterator -> Symbol_iterator ... those are polyfills. rxSubscriber is just an RxJS library-specific symbol. It wouldn't exist on window.Symbol or the like.

hmm ok I'll just leave it as rxSubscriberSymbol

…ort Babel UMD and others To migrate the code follow the example bellow: Before: import { $$observable } from 'rxjs/symbol/observable'; After: import { observable } from 'rxjs/symbol/observable'; Closes ReactiveX#2415

…Babel UMD and others To migrate the code follow the example bellow: Before: import { $$iterator } from 'rxjs/symbol/iterator'; After: import { iterator } from 'rxjs/symbol/iterator';

…support Babel UMD and others To migrate the code follow the example bellow: Before: import { $$rxSubscriber } from 'rxjs/symbol/rxSubscriber'; After: import { rxSubscriber } from 'rxjs/symbol/rxSubscriber';

coveralls · 2017-03-16T20:50:38Z

Coverage increased (+0.001%) to 97.689% when pulling bc852fa on DzmitryShylovich:gh/2415 into e4e7a18 on ReactiveX:master.

lock · 2018-06-06T12:40:43Z

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.

DzmitryShylovich mentioned this pull request Mar 3, 2017

Consider rename observable symbol $$observable => observable #2415

Closed

benlesh requested changes Mar 15, 2017

View reviewed changes

DzmitryShylovich force-pushed the gh/2415 branch from 5c868fe to da5a95d Compare March 15, 2017 20:57

benlesh approved these changes Mar 15, 2017

View reviewed changes

benlesh requested changes Mar 15, 2017

View reviewed changes

DzmitryShylovich mentioned this pull request Mar 15, 2017

fix(core): use presence of .subscribe to detect observables rather than Symbol.observable angular/angular#15171

Merged

DzmitryShylovich changed the title ~~refactor(Symbol): deprecate $$observable and replace it with observable~~ Fix symbols to work with Babel UMD exporting Mar 15, 2017

DzmitryShylovich force-pushed the gh/2415 branch from da5a95d to 62e70b6 Compare March 15, 2017 23:01

DzmitryShylovich force-pushed the gh/2415 branch from 62e70b6 to bc852fa Compare March 15, 2017 23:15

benlesh requested changes Mar 16, 2017

View reviewed changes

DzmitryShylovich added 3 commits March 16, 2017 23:36

fix(symbol/iterator): will be exported as iterator to better support …

c86a40d

…Babel UMD and others To migrate the code follow the example bellow: Before: import { $$iterator } from 'rxjs/symbol/iterator'; After: import { iterator } from 'rxjs/symbol/iterator';

fix(symbol/rxSubscriber): will be exported as rxSubscriber to better …

234442e

…support Babel UMD and others To migrate the code follow the example bellow: Before: import { $$rxSubscriber } from 'rxjs/symbol/rxSubscriber'; After: import { rxSubscriber } from 'rxjs/symbol/rxSubscriber';

DzmitryShylovich force-pushed the gh/2415 branch from bc852fa to 234442e Compare March 16, 2017 20:43

benlesh approved these changes Mar 16, 2017

View reviewed changes

benlesh added the Next Patch Version label Mar 16, 2017

benlesh merged commit 747bef6 into ReactiveX:master Apr 3, 2017

DzmitryShylovich deleted the gh/2415 branch April 3, 2017 19:57

lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix symbols to work with Babel UMD exporting #2435

Fix symbols to work with Babel UMD exporting #2435

DzmitryShylovich commented Mar 3, 2017 •

edited

Loading

coveralls commented Mar 3, 2017

benlesh left a comment

benlesh Mar 15, 2017

DzmitryShylovich Mar 15, 2017 •

edited

Loading

benlesh commented Mar 15, 2017

DzmitryShylovich commented Mar 15, 2017

coveralls commented Mar 15, 2017

benlesh commented Mar 15, 2017

benlesh left a comment

benlesh commented Mar 15, 2017

coveralls commented Mar 15, 2017

DzmitryShylovich commented Mar 15, 2017

coveralls commented Mar 15, 2017

benlesh Mar 16, 2017

benlesh Mar 16, 2017

DzmitryShylovich Mar 16, 2017

DzmitryShylovich Mar 16, 2017

benlesh Mar 16, 2017

DzmitryShylovich Mar 16, 2017 •

edited

Loading

DzmitryShylovich Mar 16, 2017

coveralls commented Mar 16, 2017 •

edited

Loading

lock bot commented Jun 6, 2018

Fix symbols to work with Babel UMD exporting #2435

Fix symbols to work with Babel UMD exporting #2435

Conversation

DzmitryShylovich commented Mar 3, 2017 • edited Loading

coveralls commented Mar 3, 2017

benlesh left a comment

Choose a reason for hiding this comment

benlesh Mar 15, 2017

Choose a reason for hiding this comment

DzmitryShylovich Mar 15, 2017 • edited Loading

Choose a reason for hiding this comment

benlesh commented Mar 15, 2017

DzmitryShylovich commented Mar 15, 2017

coveralls commented Mar 15, 2017

benlesh commented Mar 15, 2017

benlesh left a comment

Choose a reason for hiding this comment

benlesh commented Mar 15, 2017

coveralls commented Mar 15, 2017

DzmitryShylovich commented Mar 15, 2017

coveralls commented Mar 15, 2017

benlesh Mar 16, 2017

Choose a reason for hiding this comment

benlesh Mar 16, 2017

Choose a reason for hiding this comment

DzmitryShylovich Mar 16, 2017

Choose a reason for hiding this comment

DzmitryShylovich Mar 16, 2017

Choose a reason for hiding this comment

benlesh Mar 16, 2017

Choose a reason for hiding this comment

DzmitryShylovich Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

DzmitryShylovich Mar 16, 2017

Choose a reason for hiding this comment

coveralls commented Mar 16, 2017 • edited Loading

lock bot commented Jun 6, 2018

DzmitryShylovich commented Mar 3, 2017 •

edited

Loading

DzmitryShylovich Mar 15, 2017 •

edited

Loading

DzmitryShylovich Mar 16, 2017 •

edited

Loading

coveralls commented Mar 16, 2017 •

edited

Loading