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

Version comparison inadequately compares strings #30

Closed
anomiex opened this issue Jul 28, 2021 · 5 comments · Fixed by #67
Closed

Version comparison inadequately compares strings #30

anomiex opened this issue Jul 28, 2021 · 5 comments · Fixed by #67
Labels
bug Something isn't working

Comments

@anomiex
Copy link

anomiex commented Jul 28, 2021

Test file:

[].includes( 'x' );

Browserslist: safari >= 14, ios_saf >= 14

Expected output:

es-compat: checking compatibility for targets { safari_ios: '14.0-14.4', safari: '14' }

No issues found. Files are compatible with the target runtimes.

Actual output:

es-compat: checking compatibility for targets { safari_ios: '14.0-14.4', safari: '14' }

/tmp/foo.js
  1:1  error  ES2016 method 'Array.prototype.includes' is forbidden  ecmascript-compat/compat

✖ 1 problem (1 error, 0 warnings)

Notes:
The dataset used reports that the feature was added in version 9 of both browsers. While 14 > 9 is true, '14' > '9' is false.

@robatwilliams robatwilliams added the bug Something isn't working label Jul 28, 2021
@robatwilliams robatwilliams changed the title Version comparison is buggy Version comparison inadequately compares strings Jul 28, 2021
lamansky added a commit to lamansky/es-compat that referenced this issue Mar 5, 2022
@dossy
Copy link

dossy commented Mar 6, 2022

@lamansky I tried testing this using gitpkg.now.sh, and it doesn't fix the problem for me. How were you able to test this and confirm that it fixes the problem for you?

$ yarn add -D 'https://gitpkg.now.sh/lamansky/es-compat/packages/eslint-plugin-ecmascript-compat?fix-issue-30'

Editing to add: For some reason, VS Code's built-in eslint didn't pick up this change, but when executing yarn lint from the command line to run eslint, it appears to fix the issue. Nice!

@robatwilliams
Copy link
Owner

#46 and #55 were proposed for this

@robatwilliams
Copy link
Owner

@robatwilliams
Copy link
Owner

There is also an issue where it does the sort for oldest versions in targetRuntimes.js

@robatwilliams
Copy link
Owner

Released https://github.com/robatwilliams/es-compat/releases/tag/v3.0.0

with fix for this

FYI @anomiex @dossy @lamansky @NoelDeMartin @NikolayFrantsev @ka2n

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants