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

Option validation #384

Closed
platinumazure opened this issue Jun 14, 2018 · 11 comments

Comments

Projects
None yet
5 participants
@platinumazure
Copy link
Member

commented Jun 14, 2018

It could be useful to add option validation to espree and make sure that incoming options match a schema.

Are there any downsides? Do we want to allow variadic options? Should we make this validation itself opt-in?

(Inspired by eslint/eslint#10479.)

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Jun 14, 2018

One potential downside is that ESLint occasionally passes new options to all parsers to allow for feature detection (e.g. eslintScopeManager and eslintVisitorKeys). If we throw an error for unrecognized option, then we might end up with unexpected crashes when ESLint adds a new option to pass to parsers.

@platinumazure

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2018

@not-an-aardvark Good call, I didn't think of that.

Maybe we could have a solution that allows some option wildcards (e.g., if the key starts with "eslint") but otherwise raise an error on unknown options?

@tapaswenipathak

This comment has been minimized.

Copy link

commented Jul 26, 2018

👋 @platinumazure @not-an-aardvark want to add this, working on a pr for this, do you think it would be good to implement a schema like this? What specific options should be present in schema, what should be the error messages? great if you can add some more details in this issue.

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Oct 24, 2018

@tapasweni-pathak Sorry we missed this. I think we need to discuss this change some more, but we'd love your contribution if you're still interested!

@not-an-aardvark @platinumazure Another option could be to only validate keys we know (and have documented), rather than introducing additional complexity around special key name patterns.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Oct 25, 2018

To clarify, do you mean we would validate the values of known options (e.g. making sure ecmaVersion is a number rather than a string) but we would continue to ignore unknown options? That sounds fine to me -- it seems like a clear improvement over what we do now.

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Oct 27, 2018

That's correct.

@nzakas

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

Does anyone want to champion this and write up a full proposal? It's been open for several months so we should make a decision on whether or not to move forward.

@platinumazure

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

I don't want to pursue this at this point. Happy to close in a couple of days if nobody wants to take this over before then.

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Mar 10, 2019

I'm not sure what a "full proposal" would entail here since the proposed change seems to be quite small. For the sake of iteration (and to illuminate any complexities that I might be missing), what questions would be left unanswered by following one-sentence proposal?

Espree should throw an error when passed an ecmaVersion value which is neither undefined nor a number.

@nzakas

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

In this case, a full proposal means, "can someone explain what everyone has agreed to?" In an issue with back and forth comments asking questions, the true proposal can get lost (part of why we have RFCs now).

If everyone agrees that your one sentence summarizes the proposal, then we can move forward.

aladdin-add added a commit to aladdin-add/espree that referenced this issue Mar 14, 2019

Breaking: validate parser options (fixes eslint#384)
it will throw an error if any of the conditions is true:

+ ecmaVersion is invalid
+ sourceType is invalid
+ ecmaVersion < 6 & sourceType: "module"
@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

This proposal was accepted in yesterday's TSC meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.