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

runtime: Use [[Define]] for the Symbol.toStringTag property #399

Closed

Conversation

ExE-Boss
Copy link
Contributor

@ljharb
Copy link
Contributor

ljharb commented Jul 21, 2020

I filed #400 which uses a conditional; defineProperty won't work in IE < 9, which is an important target for this package.

@benjamn benjamn closed this in #400 Jul 21, 2020
@ExE-Boss
Copy link
Contributor Author

This will work in IE < 9, since it only uses defineProperty if the symbol property already exists in GeneratorPrototype’s prototype chain.

@benjamn
Copy link
Collaborator

benjamn commented Jul 21, 2020

That's a fair point @ExE-Boss! Do you see value in overriding the existing string, if it's already defined? PR #400 lets the existing string remain unchanged, whereas defineProperty would presumably change it. I have no strong opinion here.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Jul 21, 2020

Well, PR #400 makes it so that once engines start shipping Iterator.prototype[Symbol.toStringTag], then Object.prototype.toString will return [object Iterator] instead of [object Generator] for regenerator’s generator objects.

@ljharb
Copy link
Contributor

ljharb commented Jul 22, 2020

That sounds like the right behavior to me, since that's what the standard will dictate.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Jul 22, 2020

But the %Symbol.toStringTag% property of %GeneratorFunction.prototype.prototype% is "Generator", which PR #400 ends up breaking for polyfilled generators.

@ljharb
Copy link
Contributor

ljharb commented Jul 22, 2020

I'm a bit confused.

This change was motivated by a web compat concern, where Gp[Symbol.toStringTag] was being assigned, but this was breaking it. Are you saying that it's because it should be defining an own property instead of using [[Set]] and hitting a non-writable property up the [[Prototype]] chain?

@ExE-Boss
Copy link
Contributor Author

Are you saying that it's because it should be defining an own property instead of using [[Set]] and hitting a non-writable property up the [[Prototype]] chain?

Yes.

@ljharb
Copy link
Contributor

ljharb commented Jul 22, 2020

In that case, i think the helper @benjamn added should be modified to conditionally use Object.defineProperty, when it's available. It will need to be used in a try/catch, since IE 8 has the method, but it throws on things that aren't HTML elements.

@ExE-Boss
Copy link
Contributor Author

That’s what I’m doing in #402.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants