Upgrade: ajv@^6.0.1, still using json schema draft 04

[platinumazure]

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
[ ] Add something to the core
[x] Other, please explain: Upgrade ajv dependency (JSON schema validator)

What changes did you make? (Give an overview)

Upgraded ajv to ^6.0.1. Also had to change the way the Ajv instance was initialized per the release notes.

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

Not really.

I want to explain why I'm doing this: At work, we have to commit node modules to TFS (yes, horrible practice, but it's a company policy). And ajv@5.x had some files which had a leading $ character in the filename, which doesn't play well with TFS. So as a result, we've been stuck on eslint@3.x because we couldn't use ajv. But ajv@6.x renamed those files. So if this is merged in and there aren't any issues, then I can finally upgrade ESLint at work.

There's absolutely no rush on my end for merging this in, so I would be okay with waiting until after the release that is coming up (soon after the time I post this) and maybe just merge this for 4.17.0, in case we want to do any extensive tests on rule schemas. That said, npm test does pass with this version of ajv, so we should have some confidence that this is a fairly safe change.

[not-an-aardvark]

I'm a bit worried that there might be breaking changes here, but I'm fine with merging it for now -- we can revert it if it turns out that there was a breaking change.

[platinumazure]

I'm okay with waiting until after the 4.16 patch cycle so we have lots of time to look for issues. That's why I haven't merged this one.

…

On Jan 19, 2018 7:03 PM, "Teddy Katz" ***@***.***> wrote: I'm a bit worried that there might be breaking changes here, but I'm fine with merging it for now -- we can revert it if it turns out that there was a breaking change. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9856 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWeioDeEZFKV1CEDQJj-QsihunDaijks5tMTtNgaJpZM4RiXE9> .

[platinumazure]

FYI: I'm still trying to get eslint-canary working so I can verify this change in some downstream projects. That's why I added do not merge label.

[MS-elug]

Hello,
Due to a recent update of the table dependency (https://github.com/gajus/table), eslint is making the "npm install" instruction fails because of a unmeet peer dependency.
eslint requires "ajv": "^5.3.0" whereas table needs "ajv": "^6.0.1".

I'm in favor to approve the PR merge to fix this issue.

[platinumazure]

Thanks! While this is being discussed, I think we could accept a PR to pin the table dependency version here to remove that warning.

…

On Feb 26, 2018 11:52, "Mathieu Seiler" ***@***.***> wrote: Hello, Due to a recent update of the table dependency (https://github.com/gajus/ table), eslint is making the "npm install" instruction fails because of a unmeet peer dependency. eslint requires "ajv": "^5.3.0" whereas table needs "ajv": "^6.0.1". I'm in favor to approve the PR merge to fix this issue. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9856 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARWei9jruJImXHIEQGemReBF2wX81bwks5tYuEYgaJpZM4RiXE9> .

[MS-elug]

Created PR #10022 to pin the version of "table" until this discussion is completed.

[platinumazure]

Apologies for taking so long with this. I've been struggling to validate the change against downstream projects and it's really important that we don't break those. (And I also have been on holiday for the last week and a half!)

[1999]

It would also be great to update "table" dependency with this change because it depends on ajv@5 which adds additional complexity + npm warnings like "no ajv@5 installed from peerDependencies"

Thanks for PR @platinumazure 👍

[platinumazure]

@1999 Good call, I've pushed a commit to allow us to use table@^4.0.3.

[platinumazure]

(Note to TSC meeting moderator: Might be best to copy the raw markdown when presenting this issue in the meeting)

TSC Summary: This is to upgrade ajv, our schema validator (originally introduced with ESLint 4.x). It's a major upgrade but we're still using JSON Schema Draft 4 (i.e., it should accept the same schemas it currently does). npm test passes with this change.

I've also run eslint-canary with this change and got unchanged results. (N.B. The redux error is not caused by the ajv upgrade.)

This also unpins our table dependency, since the peer deps should now be compatible.

TSC Agenda: Should we accept this upgrade? And if so, should this be treated as semver-patch or semver-major?

[platinumazure]

Successfully tested this change on these repositories:

Via unit tests + RuleTester:

• ESLint

Via eslint-canary:

• body-parser
• jQuery
• mocha
• node
• Vue
• webpack

Via RuleTester:

• eslint-plugin-qunit

[mikermcneil]

Please let me know if there's any way I can help.

aside / rant:

(apologies in advance)

I'm now seeing the issue...

This upgrade aside, seems sort of insane that this is happening-- and it definitely shouldn't require an upgrade (I understand why you're doing it though, I saw for myself....)

ajv@6 is being installed in node_modules/table/node_modules/ajv, as expected. But ajv-keywords is being installed at the root node_modules folder... because flatter is better, or something? This never would have been an issue in NPM 2.

Am I missing something about the expected behavior of peer dependencies? Maybe it's time for us to expect something else.

[epoberezkin]

@platinumazure @not-an-aardvark It may be better to use the option schemaId: "auto" here (it was the default in the previous version). It would simplify migrating schemas to draft-06/07, as they will be able to co-exist. Although it can be changed later when needed.

I also hope that the issue with missing peer dependency that a lot of people report to ajv (ajv-validator/ajv#708, ajv-validator/ajv-keywords#56) could go away after this upgrade. Even though it still looks like npm problem to me (or some other tool problem), I don't know what else apart from eslint still uses ajv 5.x to cause this version conflict...

[platinumazure]

@epoberezkin

It may be better to use the option schemaId: "auto" here (it was the default in the previous version). It would simplify migrating schemas to draft-06/07, as they will be able to co-exist. Although it can be changed later when needed.

I'm not sure I understand what that does. Could you please provide a quick explanation or link me to some documentation? Thanks!

I also hope that the issue with missing peer dependency that a lot of people report to ajv (ajv-validator/ajv#708, ajv-validator/ajv-keywords#56) could go away after this upgrade. Even though it still looks like npm problem to me (or some other tool problem), I don't know what else apart from eslint still uses ajv 5.x to cause this version conflict...

It's an npm 3+ problem at the end of the day, but npm folks have said that they basically would have to rewrite their dependency tree builder to solve this (i.e., they're aware it's a bug but the effort to fix is massive). I'd have to dig up the issue again, though.

[epoberezkin]

I'm not sure I understand what that does. Could you please provide a quick explanation or link me to some documentation? Thanks!

JSON Schema draft-06 replaces the keyword “id” as the schema identifier with the keyword “$id”. So, draft-04 schemas should use “id” (and ignore “$id”) and draft-06/07 schemas should use “$id” (and ignore “id”). Because there are many schemas that don’t have $schema to identify whether they are draft-04 or draft-06 (and also some schemas with non-standard $schema), ajv uses an option “schemaId” to decide which ID keyword to use.

The default behaviour in ajv 5.x was to use both “id” and “$id”, whatever is present (and throw exception if both are present and different - not a likely case).
The default behaviour in ajv 6.x is to use only “$id”, but with option schemaId: “auto” it does what 5.x does by default, which is supportive of the gradual migration of schemas to draft-06/07 (which I think would be a good idea as there are many useful additional validation keywords).

schemaId: “id” (the current PR) will only work with draft-04 schemas, that potentially may break something if there are some draft-06/07 schemas somewhere out there.

Thanks a lot for the comment about npm, I was not certain if it is npm issue.

[platinumazure]

@epoberezkin Thanks for the explanation. Sounds like it only really makes a difference if "id" or "$id" are used-- I assume that's mostly for when you define a schema that you want to reference somewhere else? In our case, our core rule schemas are standalone and as far as I can tell we don't give them identifiers at all, although I can't say the same for plugin developers outside of the core ESLint ecosystem.

Given that schemaId: "auto" is supposed to have the same behavior as ajv 5.x, that probably means that's what I want anyway-- I definitely didn't intend to further limit schema compatibility with this PR. I will push a commit to fix shortly.

[txwizard]

For what it's worth, I just ran npm list ajv against a fresh React installation, which gave me the following report.

npm list ajv
material_ui_table_demo@0.1.0 F:\Praesidium\Armatus_Admin_EventRegistration\React\material_ui_table_demo
`-- react-scripts@1.1.2
  +-- eslint@4.10.0
  | `-- ajv@5.5.2
  +-- extract-text-webpack-plugin@3.0.2
  | `-- schema-utils@0.3.0
  |   `-- ajv@5.5.2  deduped
  +-- UNMET OPTIONAL DEPENDENCY fsevents@1.1.3
  | `-- UNMET OPTIONAL DEPENDENCY node-pre-gyp@0.6.39
  |   `-- UNMET OPTIONAL DEPENDENCY request@2.81.0
  |     `-- UNMET OPTIONAL DEPENDENCY har-validator@4.2.1
  |       `-- UNMET OPTIONAL DEPENDENCY ajv@4.11.8
  +-- jest@20.0.4
  | `-- jest-cli@20.0.4
  |   `-- jest-environment-jsdom@20.0.3
  |     `-- jsdom@9.12.0
  |       `-- request@2.85.0
  |         `-- har-validator@5.0.3
  |           `-- ajv@5.5.2  deduped
  `-- webpack@3.8.1
    `-- ajv@5.5.2  deduped

In addition to eslint, I see two other significant ajv dependencies: extract-text-webpack-plugin@3.0.2 and webpack@3.8.1.

Unless I see something about breaking changes in the changelog, I plan to upgrade to ajv 6.

[platinumazure]

I've rebased with our other package.json changes, so hopefully this is good to go.

@eslint/eslint-tsc This change has been pretty borderline on whether it needs TSC approval (it's a dependency upgrade, which usually doesn't need TSC approval, but it also was possibly breaking). Since then, the maintainer of ajv has dropped by and helped me correct one small thing, so I think we are now good and fully backward-compatible. I'd like to merge this before the next prerelease, unless someone from TSC objects and then it can stay on the agenda.

I really think this should go in a prerelease so that the community can help try this out before we do the final 5.0.0 release, and we can make changes if needed (including possibly upstream in ajv, in the worst case). I'm sure the TSC would agree with that in spirit, but we've had trouble achieving quorum lately and I don't want this to sit for too much longer.

I'll give it until the next TSC meeting, and then remove "tsc agenda" label and merge on 10-11 April, unless one of the following happens:

• We have quorum at TSC meeting and this is approved as a normal motion (yay), in which case I'll merge;
• All TSC members approve the PR, 👍 this comment, or 👍 the initial PR post (I'll be union-ing those, no need to leave reactions on both), in which case I'll merge when I see that; or
• One TSC member leaves a 👎 here or on the initial PR post, or leaves a comment that is clearly an objection of some kind, in which case I won't merge and will wait for TSC consensus in a TSC meeting as normal (including waiting until we have a meeting where we achieve quorum).

I hope this is a reasonable compromise.

[not-an-aardvark]

This issue was accepted in today's TSC meeting.