Skip to content

Avoid uses of ES6 Set, use Array instead #28921

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

Merged
merged 1 commit into from
Dec 11, 2018

Conversation

ajafff
Copy link
Contributor

@ajafff ajafff commented Dec 8, 2018

Fixes: #28918

/cc @Andy-MS

@arktronic
Copy link

This looks like a step in the right direction for fixing #28918, but I believe more needs to be done for that issue. For example, inspectValue.ts also uses String.prototype.startsWith, which is another function introduced in ES6. I'm unsure of how many other ES5 incompatibilities exist in this and/or in other files.

@NN---
Copy link

NN--- commented Dec 9, 2018

Why just not use core-js or any other polyfill ?

@arktronic
Copy link

Given the fact that functions like ts.startsWith exist in the codebase, it would appear that TypeScript is intended to run in a pre-ES6 environment without polyfills. Official guidance on this from project maintainers would be helpful, of course.

@j-oliveras
Copy link
Contributor

I opened #28932 to solve the String.prototype.startsWith and some others.

@weswigham
Copy link
Member

We probably used Set because we assumed we had self-polyfilled it like we do Map - it's probably best to write out the polyfill instead.

@ajafff
Copy link
Contributor Author

ajafff commented Dec 10, 2018

it's probably best to write out the polyfill instead.

What's the benefit of that? There are still other ES6 features without a polyfill that could easily be used by accident (see #28933).
Also note that Set doesn't give any additional performance and isn't easier to use in these cases.

@weswigham
Copy link
Member

What's the benefit of that?

Using the platform Set when it's available, mostly (which normally has better memory characteristics, at least). Like what we do with Map would be ideal.

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

Successfully merging this pull request may close these issues.

6 participants