Skip to content

Generator helper should include [Symbol.toStringTag] #35833

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

Open
sirreal opened this issue Dec 23, 2019 · 8 comments
Open

Generator helper should include [Symbol.toStringTag] #35833

sirreal opened this issue Dec 23, 2019 · 8 comments
Assignees
Labels
Needs Investigation This issue needs a team member to investigate its status.

Comments

@sirreal
Copy link

sirreal commented Dec 23, 2019

TypeScript Version: 3.8.0-dev.20191223 and 3.7.3

Search Terms:

Generator, toStringTag, transformer, helper, tslib

Code

The following code will error in when using compilerOptions.target = es5, but works correctly when compiled with target = es2015 or greater.

// index.ts
function* f() {}
if (f()[Symbol.toStringTag] !== 'Generator') {
	  throw new Error("Bad generator!")
}
console.log('👍 Works well!');
tsc --target es5 --lib dom,es5,es2015 ./index.ts && node index.js
# Error: Bad generator!
tsc --target es2015 ./index.ts && node index.js
# 👍Works well!

Expected behavior:

The property should be set as expected via the helpers.

Generators should include the Symbol.toStringTag property "Generator" according to spec.

Some libraries check for a generator by inspecting this property. For example, @wordpress/redux-routine checks for this property and does not correctly handle generators produced by the helper.

regenerator-runtime is a widely used generator helper which does set this property.

Actual behavior:

Symbol.toStringTag is not included on the generator.

Playground Link:

Broken with target=es5
Working with target=es2015

Related Issues:

#19006

@DanielRosenwasser
Copy link
Member

This is so bizarre to me - why on earth would code want to defensively check like this when generators are not the only way to produce iterators?

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Dec 24, 2019

Do note that it should also inherit from %IteratorPrototype%, which will be exposed by the Iterator helpers proposal as Iterator.prototype.

@sirreal
Copy link
Author

sirreal commented Jan 7, 2020

I'd be happy to open a PR for this issue, the fix seems clear. Contribution guidelines suggest I should wait for a "help wanted" label.

Would folks be open to that contribution?

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jan 14, 2020
@RyanCavanaugh
Copy link
Member

@rbuckton thoughts?

@rbuckton
Copy link
Member

Do note that it should also inherit from %IteratorPrototype%, which will be exposed by the Iterator helpers proposal as Iterator.prototype.

There is no %IteratorPrototype% down-level, and even if the iterator helpers proposal advances, I don't believe we would attempt to ship a polyfill for that behavior. Also, our helpers can often run in isolated contexts (i.e. inside of a single module), so there's no guaranteed way to create a shared %IteratorPrototype%

I concur with @DanielRosenwasser, just having a Symbol.toStringTag doesn't make something an generator. Our downlevel emit is intended to support the syntactic behavior of generators. I don't know that we should go so far as to fully emulate the entire runtime behavior of generators (with respect to Symbol.toStringTag and %IteratorPrototype%).

Rather, I wonder at the reason why @wordpress/redux-routine is depending on Symbol.toStringTag, rather than Symbol.iterator.

@sirreal
Copy link
Author

sirreal commented Jan 15, 2020

just having a Symbol.toStringTag

To clarify, the check in @wordpress/redux-routine is maybeGenerator[ Symbol.toStringTag ] === 'Generator'. That does seem to be a valid way of checking for native generators.

[wondering why a library would depend] on Symbol.toStringTag, rather than Symbol.iterator

This been mentioned a few times, is the following the type of check you would suggest?

function isGenerator(x) {
  return !!x && typeof x[Symbol.iterator] === 'function';
}

function* makeGen() { }
const gen = makeGen();
console.log(isGenerator(gen)); // true

This check does seem to be satisfied by native, regenerator-runtime, and the TypeScript down-level implementation. I can propose that change to @wordpress/redux-routine.

FYI: @aduth

@DanielRosenwasser
Copy link
Member

Yeah, I think that's probably a more appropriate check, though you'd want to name the function something like isIterable() if you did that. Double check with @aduth though of course, since I don't want to assume the wrong intent as an outsider.

sirreal added a commit to WordPress/gutenberg that referenced this issue Jan 16, 2020
Rather than checking the `Symbol.toStringTag` property, check for
`iterator` and `iterable` interfaces that are more widely supported by
helper libs.

Related issue:
microsoft/TypeScript#35833
@sirreal
Copy link
Author

sirreal commented Jan 23, 2020

In WordPress/gutenberg#19666 we changed the check in @wordpress/redux-routine to typeof object[ Symbol.iterator ] === 'function' && typeof object.next === 'function'. This is effectively testing the generator interface.

@wordpress/redux-routine expects to operate on action objects. While an array of objects might make sense, one example that clearly doesn't make sense here but satisfies the Symbol.iterator check is strings, i.e. typeof "foo"[Symbol.iterator] === 'function'. The library doesn't want to treat the characters of a string as if they were yielded actions.


Thanks for the feedback everyone. With the change in @wordpress/redux-routine, my needs are satisfied. I'll leave this to maintainers to decide whether there's anything to more to consider here or if you'd like to close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

No branches or pull requests

5 participants