refactor: replace is-my-json-valid with Ajv #8852

gajus · 2017-07-01T11:10:31Z

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[X] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Replace the ESLint rule validation library from is-my-json-valid to Ajv.

Is there anything you'd like reviewers to focus on?

WIP. Will ping when ready for review.

Motivation

Ajv has more contributors, more downloads, more regular releases. https://npmcompare.com/compare/ajv,is-my-json-valid summarizes it quite well.

From the technical perspective, the Ajv is the fastest and the most standard compliant (https://github.com/epoberezkin/ajv#performance) implementation of a JSON validator.

It is being used in webpack. I am using it in https://www.npmjs.com/package/table, https://www.npmjs.com/package/isomorphic-webpack, etc.

Finally, Ajv was originally planned to be used for ESLint (#8295 (comment)). The current error message formatting is a bit hacky as a result. Ajv (optionally) provides a custom error message abstraction via https://github.com/epoberezkin/ajv-errors.

eslintbot · 2017-07-01T11:10:32Z

Thanks for the pull request, @gajus! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

mention-bot · 2017-07-01T11:10:34Z

@gajus, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vitorbal, @not-an-aardvark and @gyandeeps to be potential reviewers.

eslintbot · 2017-07-01T18:17:50Z

Thanks for the pull request, @epoberezkin! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

jsf-clabot · 2017-07-01T18:17:50Z

All committers have signed the CLA.

eslintbot · 2017-07-01T19:15:09Z

Thanks for the pull request, @epoberezkin! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

platinumazure

I assume this is still WIP so I didn't do a full review- just noticed one tiny thing you might want to fix up in the next draft. Thanks!

platinumazure · 2017-07-01T19:22:17Z

lib/util/ajv.js

+});
+
+ajv.addMetaSchema(metaSchema);
+/* eslint no-underscore-dangle: "off" */


I believe this will disable the rule for the whole file. I think you might want to use // eslint-disable-next-line no-underscore-dangleinstead?

thank you, I updated

eslintbot · 2017-07-01T19:43:21Z

Thanks for the pull request, @epoberezkin! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

eslintbot · 2017-07-01T19:56:29Z

Thanks for the pull request, @epoberezkin! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

eslintbot · 2017-07-01T20:13:33Z

Thanks for the pull request, @epoberezkin! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

eslintbot · 2017-07-01T20:39:00Z

Thanks for the pull request, @epoberezkin! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

eslintbot · 2017-07-01T20:57:16Z

LGTM

eslintbot · 2017-07-01T21:48:17Z

LGTM

not-an-aardvark · 2017-07-02T07:54:19Z

lib/rules/comma-dangle.js

-                    valueWithIgnore: {
-                        anyOf: [
-                            {
-                                $ref: "#/defs/value"


I haven't looked at this thoroughly yet, but as a quick question: Would this be a breaking change for rules? If some third-party rules have been relying on the (apparently nonstandard) $ref property from is-my-json-valid, it seems like that won't work anymore if $ref is no longer supported in ajv.

@not-an-aardvark this change only fixes the schema, it is not related to ajv. Ajv fully supports $ref (and it is standard), the changed schema also has $ref's. Should it be in a separate PR maybe? @gajus?

Oh I see, I misunderstood what the change was here. I guess the more general question is: is this schema fix necessary, or would the PR still work without this schema fix? (I don't mind modifying the schema in general, but if it requires changes then that might indicate that switching to Ajv would risk breaking third-party rules.)

@not-an-aardvark @gajus, now I understand the problem here. When you pass array as a rule schema (schema: [obj], where obj as a JSON schema) eslint internally converts it to a following JSON schema:

{ type: 'array', items: [ obj ], minItems: 0, maxItems: 1 }

If there are definitions inside obj (like it is with comma-dangle) then their JSON pointers should be extended as well, otherwise they are not found. is-my-json-valid quietly ignores $ref's that are not found and ajv (by default) throws exception. This change to the schema fixes this issue. In addition there are "$refs" instead of "$ref" in several places, that are ignored by both is-my-json-valid and by ajv.

There may be some third party rules with similar incorrect usage of definitions not on the top level but inside item schema and these rules will start logging errors. Should it be a breaking change? I am not sure. These rules are not working correctly now so maybe it is ok if they have to be fixed. There are the following options:

Leave things as they are (therefore breaking third-party rules with incorrect schemas)

Change Ajv settings to log warnings when $ref's are not found instead of throwing exception, so things will work as they do now and all rules can be fixed separately (including comma-dangle).

Change Ajv settings to keep things completely as they are, quietly ignoring $ref's that cannot be found.

Improve eslint to correctly extend JSON pointers in the above scenario, so internally "#/defs/value" would become "#/items/0/defs/value". That's my least favourite option, as it can be confusing to debug and some third-party rules can still start breaking as currently they simply don't validate against these schemas and then they start, so if something was invalid it would start breaking.

I think option 2 is the safest, but let me know what you think.

That makes sense, thanks for explaining. I agree that option 2 seems safest, if it's not too difficult a change. Maybe we can switch to option 1 in a future major release. Since a core rule had this problem but was otherwise working fine, it might not be safe to assume that the third-party rules with this problem would need to be fixed anyway.

Thank you.
Option 2 is a very simple change, I will do it. And I will move fix for comma-dangle in a separate PR.
Option 4 could be a good change for the next major version as users probably expect that all items are validated separately and therefore use $ref's, expecting them to work, rather than ignored. So instead of saying "you can't use $ref if your rule schema is an array" or "you have to understand internal mechanics of eslint to use $ref in such cases" we can just make things work as users expect, either by validating items separately (as users think eslint does) or by transforming schemas correctly taking $ref's into account.

It is done, fix to comma-dangle and docs update on $ref usage are in #8864

@epoberezkin

This PR replaces the validation functionality. @epoberezkin needs to have a look into how to handle the error message formatting.

eslintbot · 2017-07-02T16:13:22Z

LGTM

gajus · 2017-07-02T17:51:41Z

LGTM

qfox · 2017-07-05T22:02:43Z

lib/config/config-validator.js

@@ -181,10 +186,10 @@ function formatErrors(errors) {
 * @returns {void}
 */
 function validateConfigSchema(config, source) {
-    const validator = schemaValidator(configSchema, { verbose: true });
+    validateSchema = validateSchema || ajv.compile(configSchema);


Personally I'd put this function to block code with validateSchema variable like:

{ let validateSchema; /** * ... */ function validateConfigSchema(...) { validateSchema = validateSchema || ajv.compile(configSchema); // ... } }

It would help to read this part of code as well as help v8 to reduce variables count in context.

Or at least just put it closer to this function.

p.s. Not a blocker.

@zxqfox is it better to make a commit now or make a separate PR after this one merged?

I'm not sure we ever need that. It's just my personal preferences. Prob no need to do that at all ;-)

There can be an eslint rule for that ;)

qfox · 2017-07-05T22:06:06Z

lib/util/ajv.js

+//------------------------------------------------------------------------------
+
+const Ajv = require("ajv"),
+    metaSchema = require("ajv/lib/refs/json-schema-draft-04.json");


There is already draft-06, hope you know.

Ajv supports draft-06 by default - this code is needed as the schemas for all rules would be draft-04 and there are changes in draft-06 that would break some of them. So I thought that migrating to draft-06 would require a major version change as some third party rules will need to change their schemas.

Ah, right. Forgot about we checking options for rules with JSON schemas. Sorry and thank you.

UPD: btw, do we want to (or can we) use "$schema": "http://json-schema.org/draft-04/schema#", field in our rules?

not-an-aardvark · 2017-07-06T06:59:55Z

I'm not sure this actually needs to be approved by the TSC since it's just an internal change, but it seems like there was some confusion about whether it needed TSC approval, so I'll add it to the agenda for tomorrow's meeting just in case.

TSC Summary: This PR proposes switching the internal schema validator to use ajv rather than is-my-json-valid.

TSC Question: Should we accept this proposal?

not-an-aardvark · 2017-07-07T01:01:07Z

This was accepted in today's TSC meeting.

not-an-aardvark · 2017-07-07T01:02:19Z

Thanks for contributing!

gajus · 2017-07-07T10:04:42Z

Thank you for accepting this promptly.

platinumazure reviewed Jul 1, 2017

View reviewed changes

epoberezkin force-pushed the master branch from 4a7b27a to c84d01a Compare July 1, 2017 19:56

epoberezkin force-pushed the master branch from c84d01a to 34137e3 Compare July 1, 2017 20:13

epoberezkin force-pushed the master branch from 34137e3 to 02acbb0 Compare July 1, 2017 20:38

epoberezkin force-pushed the master branch from 02acbb0 to afc4305 Compare July 1, 2017 20:57

not-an-aardvark reviewed Jul 2, 2017

View reviewed changes

not-an-aardvark added chore This change is not user-facing core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jul 2, 2017

gajus changed the title ~~refactor (WIP): replace is-my-json-valid with Ajv~~ refactor: replace is-my-json-valid with Ajv Jul 2, 2017

gajus and others added 8 commits July 2, 2017 17:03

Upgrade: replace is-my-json-valid with Ajv

235aaa5

This PR replaces the validation functionality. @epoberezkin needs to have a look into how to handle the error message formatting.

Fix: error message code and messages in tests

0edc7ef

Chore: use one instance of ajv, JSON-schema draft-04 compatibility

b5987ae

Chore: use meta-schema in ajv, disable default meta-schema validation

b702640

Chore: validate config-schema against meta-schema

9d7e54b

Chore: compile config-schema only once

9ca23a6

Fix: RuleTester error message for invalid schema

f19082c

Fix: ignore missing $ref in schemas for backward compatibility

df5c039

epoberezkin force-pushed the master branch from 431a570 to df5c039 Compare July 2, 2017 16:13

ilyavolodin approved these changes Jul 3, 2017

View reviewed changes

gyandeeps approved these changes Jul 3, 2017

View reviewed changes

qfox approved these changes Jul 5, 2017

View reviewed changes

not-an-aardvark added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Jul 6, 2017

not-an-aardvark approved these changes Jul 6, 2017

View reviewed changes

not-an-aardvark merged commit 72f22eb into eslint:master Jul 7, 2017

gyandeeps mentioned this pull request Jul 7, 2017

Plugin status and discussion eslint/eslint-github-bot#3

Closed

nwoltman mentioned this pull request Jul 9, 2017

react/jsx-curly-spacing rule crashes ESLint 4.2.0 jsx-eslint/eslint-plugin-react#1290

Closed

jseminck mentioned this pull request Jul 9, 2017

Schema JSON validation fails with ESLint 4.2.0 #8908

Closed

HomeZenBot mentioned this pull request Jul 10, 2017

⬆️ eslint @ 4.2.0 homezen/eslint-config-homezen#43

Merged

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

refactor: replace is-my-json-valid with Ajv #8852

refactor: replace is-my-json-valid with Ajv #8852

Conversation

gajus commented Jul 1, 2017 • edited

eslintbot commented Jul 1, 2017

mention-bot commented Jul 1, 2017

eslintbot commented Jul 1, 2017

jsf-clabot commented Jul 1, 2017 • edited

eslintbot commented Jul 1, 2017

platinumazure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jul 1, 2017

eslintbot commented Jul 1, 2017

eslintbot commented Jul 1, 2017

eslintbot commented Jul 1, 2017

eslintbot commented Jul 1, 2017

eslintbot commented Jul 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epoberezkin Jul 2, 2017 • edited

Choose a reason for hiding this comment

not-an-aardvark Jul 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jul 2, 2017

gajus commented Jul 2, 2017

qfox Jul 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epoberezkin Jul 5, 2017 • edited

Choose a reason for hiding this comment

qfox Jul 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-an-aardvark commented Jul 6, 2017 • edited

not-an-aardvark commented Jul 7, 2017

not-an-aardvark commented Jul 7, 2017

gajus commented Jul 7, 2017

gajus commented Jul 1, 2017 •

edited

jsf-clabot commented Jul 1, 2017 •

edited

epoberezkin Jul 2, 2017 •

edited

not-an-aardvark Jul 2, 2017 •

edited

qfox Jul 5, 2017 •

edited

epoberezkin Jul 5, 2017 •

edited

qfox Jul 5, 2017 •

edited

not-an-aardvark commented Jul 6, 2017 •

edited