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

Update: add latestEcmaVersion & supportedEcmaVersions #430

Merged
merged 3 commits into from Feb 19, 2020

Conversation

kaicataldo
Copy link
Member

Inspired by this discussion, this is a proposal to add latestEcmaVersion() & supportedEcmaVersions() to the public API.

Benefits:

  1. We can query the latest ecmaVersion from the parser for rules (as is mentioned in the comment listed above).
  2. The supported ecmaVersions in our demo have gotten out of sync with ESLint core. This would allow us to calculate this programmatically and not have to manually update it.
  3. This is a fairly minimal API that shouldn't require much maintenance. We will most likely adjust this code once a year.

Cons:

  1. This increases the surface area of our API and will have to be maintained.

Open questions:

  1. If we use this in core, are we expecting 3rd party parsers to also implement this? I would say no (and that we should always fall back to Espree in the event it doesn't exist on a 3rd party parser). I believe this is still better than it being hardcoded in core.

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

A couple of small things I noticed but no blockers.

lib/options.js Outdated
* Get the latest ECMAScript version supported by Espree.
* @returns {number} The latest ECMAScript version.
*/
function latestEcmaVersion() {
Copy link
Member

Choose a reason for hiding this comment

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

I’d suggest calling this getLatestEcmaVersion to indicate a value is being retrieved (verb as opposed to noun).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call! I was initially thinking of it being more of a static property (like version), but that makes more sense with this iteration 👍

lib/options.js Outdated
* Get the list of ECMAScript versions supported by Espree.
* @returns {number[]} An array containing the supported ECMAScript versions.
*/
function supportedEcmaVersions() {
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, getSupportedEcmaVersions.

lib/options.js Outdated
* @returns {number} The latest ECMAScript version.
*/
function latestEcmaVersion() {
return Math.max(...SUPPORTED_VERSIONS);
Copy link
Member

Choose a reason for hiding this comment

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

You can avoid a calculation here by using SUPPORTED_VERSIONS[SUPPORTED_VERSIONS.length - 1]

@kaicataldo
Copy link
Member Author

Thanks for the review! Updated.

@kaicataldo kaicataldo changed the title Update: add latestEcmaVersion() & supportedEcmaVersions() Update: add getLatestEcmaVersion() & getSupportedEcmaVersions() Feb 12, 2020
@kaicataldo
Copy link
Member Author

Ah, I like that! That’s more in line with my initial design and I think is a nicer API. Will update.

@kaicataldo kaicataldo changed the title Update: add getLatestEcmaVersion() & getSupportedEcmaVersions() Update: add latestEcmaVersion() & supportedEcmaVersions() Feb 13, 2020
@kaicataldo kaicataldo changed the title Update: add latestEcmaVersion() & supportedEcmaVersions() Update: add latestEcmaVersion & supportedEcmaVersions Feb 13, 2020
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

Yes! Very nice now.

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

LGTM. I can see this being useful, and I like how tiny the API addition ends up being.

@kaicataldo kaicataldo merged commit acb8776 into master Feb 19, 2020
@kaicataldo kaicataldo deleted the ecmaversion-apis branch February 19, 2020 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants