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
Closed

Option validation #384

platinumazure opened this issue Jun 14, 2018 · 11 comments

Comments

@platinumazure
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

@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
Copy link

tapaswenipathak 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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

That's correct.

@nzakas
Copy link
Member

nzakas 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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

nzakas 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
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
Copy link
Member

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
Projects
None yet
Development

No branches or pull requests

5 participants