Improve linting to check for ES5 conformance (with exceptions to allow use of ES6 classes in certain modules) #5342
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
diarmidmackenzie
changed the title
|
@dmarcos Any thoughts on this one? Personally, this would be a big help when contributing to A-Frame, in helping to ensure my code is ES5-compliant. Are there any downsides you are concerned about? |
dmarcos
reviewed
Nov 5, 2023
|
This needs rebase. Sorry for the wait. FYI, ES6 classes is not part of A-Frame code style. Forced on the code base because of the custom elements API. I don't think it matters for this PR though. |
dmarcos
reviewed
Nov 5, 2023
Previous attempt didn't preserve this in forEach() callback. This version should work better (while still only using ES5 constructs).
diarmidmackenzie
force-pushed
the
improve-linting
branch
from
bc3e20a to
6217b74
Compare
|
Rebased against latest master. Should be OK to merge now. |
|
Thanks so much for the patience and quick turnaround. You rock! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
The A-Frame codebase is intended to only use ES5 JavaScript - with the exception of ES6 classes, which are used sparingly in a few places.
Currently, the enforcement of this is manual during PR reviews, which is (a) not completely reliable, and (b) often creates unecessary reviewing cycles, particular for PRs from contributors who are used to using ES6 features.
As proposed in #5218, this PR updates linting to enforce the rule that code in /src should only use ES5 JavaScript.
In order to permit the use of ES6 classes where required, this PR exempts the small number of modules that use ES6 classes from the ES5 checks. While that's not perfect, it still enables ES5 checking for 90% of the /src tree, which should be valuable in helping contributors avoid accidentally including ES6 constructs in PRs.
Changes proposed:
Update from semistandard v9 (6 years old) to standardx v17 (latest). standardx allows us to use the semistandard config as a base, but fine-tune exactly which rules we want to apply. This also brings us up to a modern version of ESLint.
Parse all code in /src as ES5, so linting errors will occur if ES6 constructs are used. A small number of core modules that use ES6 classes are excepted, and parsed as ES6. Note that we are unable to detect ES6 usage in these modules.
Bring code into line with latest v17 semistandard rules, with just 3 classes of exceptions:
To conform with the new rules, there's some fairly trivial changes to ~25 modules, reflecting changes over the last 6 years to the semistandard conventions. I don't believe any of these will be controversial.