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.iterator): no longer polyfilled #3389

Merged
merged 1 commit into from
Mar 7, 2018

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Mar 7, 2018

BREAKING CHANGE: We are no longer polyfilling Symbol.iterator. That would be done by a proper polyfilling library

@benlesh benlesh requested review from jayphelps and kwonoj March 7, 2018 00:12
@benlesh
Copy link
Member Author

benlesh commented Mar 7, 2018

NOTE: The only worry I have with this change is that we're no longer watching for the weirdness with older versions of Firefox.. however that should be covered by a proper polyfill.

cc @IgorMinar @jasonaden

@rxjs-bot
Copy link

rxjs-bot commented Mar 7, 2018

Messages
📖

CJS: 1311.8KB, global: 698.0KB (gzipped: 114.3KB), min: 134.7KB (gzipped: 29.7KB)

Generated by 🚫 dangerJS

polyfill. We don't want to throw on this, because it's not required
by the library. However it will provide clues to users on older
browsers why things like `from(iterable)` doesn't work. */
if (!Symbol.iterator) {
Copy link
Member

@jayphelps jayphelps Mar 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe Symbol.iterator and Symbol itself were both defined in ES2015 so if Symbol.iterator isn't available it's likely that Symbol isn't either and this may error. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Good catch!

}

export const iterator = symbolIteratorPonyfill(root);
/** The native Symbol.iterator instance or a string */
export const iterator = Symbol.iterator || '@@iterator';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue here Uncaught ReferenceError: Symbol is not defined

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

BREAKING CHANGE: We are no longer polyfilling `Symbol.iterator`. That would be done by a proper polyfilling library
polyfill. We don't want to throw on this, because it's not required
by the library. However it will provide clues to users on older
browsers why things like `from(iterable)` doesn't work. */
if (!Symbol || !Symbol.iterator) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benlesh This still has the "Symbol is not defined" problem. Needed to use typeof checks. Here's a follow up PR to fix: #3394

@lock
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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants