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

Does Hermes really support ConstAndLet? #2365

Closed
tido64 opened this issue Jul 1, 2022 · 6 comments
Closed

Does Hermes really support ConstAndLet? #2365

tido64 opened this issue Jul 1, 2022 · 6 comments

Comments

@tido64
Copy link

tido64 commented Jul 1, 2022

First of all, thanks for adding the Hermes target in 4e631f5. We're maintaining a small plugin that targets Hermes, and this makes our lives so much easier.

It looks like the target is listed to support ConstAndLet, and I'm wondering what that means in terms of transpilation output. I know that let and const are listed under In Progress on their website, and the engine does parse them just fine, but I also know that it basically just interprets them as var. There is no support for block-scoped variables (facebook/hermes#575 (comment)), and you can still overwrite the values. If ConstAndLet assumes either of these, then it's probably a bug and should be removed.

@evanw
Copy link
Owner

evanw commented Jul 1, 2022

Compatibility data is managed by another project: https://github.com/kangax/compat-table. I added the data assuming it would be correct, and therefore useful. If it’s incorrect and not useful then I can remove the Hermes target from esbuild in the next breaking change release. I do not want to test for and maintain JavaScript feature compatibility matrices for obscure targets like Hermes myself.

@kelset
Copy link

kelset commented Jul 1, 2022

thanks for the info @evanw! I just noticed that over there there's a draft PR by the team at Meta maintaining the project: compat-table/compat-table#1808 so I'd say that once that's merged the data will most likely be correct

@evanw
Copy link
Owner

evanw commented Jul 1, 2022

Also it’s really strange how broken this is. Making let and const an alias for var seems worse than just only supporting var since it’s inevitable that it’ll cause problems. I wonder what other features that Hermes “supports” are also fundamentally broken like this.

@tido64
Copy link
Author

tido64 commented Jul 4, 2022

Also it’s really strange how broken this is. Making let and const an alias for var seems worse than just only supporting var since it’s inevitable that it’ll cause problems. I wonder what other features that Hermes “supports” are also fundamentally broken like this.

I completely agree with this. I still think the Hermes target is useful, so I hope it will stay. Until the PR to compat-table lands, I've submitted a PR to remove Hermes from ConstAndLet. Please let me know if this is okay, or if there's anything else you need.

@evanw
Copy link
Owner

evanw commented Jul 6, 2022

I have investigated this and determined that the bug is with esbuild's compatibility table generator, not with the compatibility data. The compatibility data correctly marks Hermes as broken regarding const and let but the script has a bug that overlooks that data.

Specifically the problem is when the test for a feature has many sub-tests and some of those sub-tests have always been broken and have never been fixed. This situation hasn't come up before because previously esbuild has only supported targets that fix their bugs (and that ship at least one version where the feature works) instead of leaving them broken in all shipped versions.

I'll fix esbuild's compatibility table generator script which will fix this problem, and which will mark a lot of features as unsupported in Hermes.

@evanw evanw closed this as completed in 18ef7be Jul 6, 2022
@tido64
Copy link
Author

tido64 commented Jul 6, 2022

@evanw Thanks for fixing this 🙏

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 a pull request may close this issue.

3 participants