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

use eslint-plugin-es5 to enforce ES5 #107

Merged
merged 3 commits into from Aug 18, 2020

Conversation

brodybits
Copy link
Member

The purpose is to avoid ES6 items that may cause issues with older engines, as discussed in PR #87.

This should guard against newer ES6 syntax that could lead to issues with some ES5 environments such as:

This PR is based on PR #106.

I would appreciate a quick review from someone, if possible.

@brodybits brodybits mentioned this pull request Aug 17, 2020
@brodybits brodybits force-pushed the add-and-use-eslint-plugin-es5 branch from 4c30ff8 to 227290d Compare August 17, 2020 22:37
Copy link
Member

@karfau karfau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

.eslintrc.yml Outdated
Comment on lines 5 to 10
extends:
# FUTURE TBD:
# - 'eslint:recommended'
- 'plugin:es5/no-es2016'
parserOptions:
ecmaVersion: 6
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question https://github.com/xmldom/xmldom/pull/106/files/2181dcbda3b0e27d9033a6b3dd988a912995d6cd#r471948818 got me thinking if we need these constraints in the top-level .eslintrc.yml, now that we have the constraint in lib/.eslintrc.yml.

I had actually wanted to enforce 'plugin:es5/no-es2015' at the top level, and only relax it in test, but I don't know how to do this and may raise an issue on the plugin. Any ideas would be awesome.

Thanks for the quick reviews. I will probably merge these later today.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I think configuring it to a more current version on the top level and only restricting it in lib makes a lot of sense, assuming we might be looking into ways to allow more modern syntax also in lib at some point in time. In that case we would only need to remove the limitation for lib.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now put ecmaVersion back to 11 and added back es2020: true in env above, like I did in PR #106, removed 'plugin:es5/no-es2016', and added explicit plugins with es5 above, hope it is OK. I put this PR in draft since I would like to merge PR #106 first.

Please let me know within the next day if you see any issues, otherwise I would like to finish with merging #106 & #107.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All very reasonable, Go for it!

@brodybits brodybits marked this pull request as draft August 18, 2020 20:21
@brodybits brodybits marked this pull request as ready for review August 18, 2020 21:46
should guard against newer ES6 syntax that could lead to issues with
some ES5 environments such as:

- Jint - https://github.com/sebastienros/jint
- Duktape
- IE11
- Android pre-5.0 WebView (already past EOL)

as discussed in PR xmldom#91 - xmldom#91
@brodybits brodybits force-pushed the add-and-use-eslint-plugin-es5 branch from 286679e to 4dbb226 Compare August 18, 2020 21:49
@brodybits brodybits changed the title eslint with eslint-plugin-es5 use eslint-plugin-es5 to enforce ES5 Aug 18, 2020
@brodybits brodybits merged commit 3609c88 into xmldom:master Aug 18, 2020
@brodybits brodybits deleted the add-and-use-eslint-plugin-es5 branch August 18, 2020 21:53
@brodybits brodybits mentioned this pull request Oct 2, 2020
This was referenced Mar 13, 2021
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.

None yet

2 participants