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

chore(config-validator): update ajv to v8 #3242

Merged
merged 1 commit into from Jun 24, 2022

Conversation

Zirak
Copy link
Contributor

@Zirak Zirak commented Jun 16, 2022

Description

ajv introduced some breaking changes from v6 to v7, and from v7 to v8. Most can
be found in their migration guide.

This commit addresses the following:

  • ErrorObject.dataPath was renamed to ErrorObject.instacePath
  • Wordings of some errors changed (e.g. "should" -> "must")
  • Strict mode does not allow a top-level default value
  • Strict tuple checking was confused by the rule array definition, see
    Unconstrained tuples
  • The plugins schema included an invalid object definition for rules
  • schemaId is no longer an ajv option
  • missingRefs no longer seems to affect the usage of ajv in the project
  • addKeyword has its signature changed

The tests pass and everyone's seemingly happy; do say if anything's still
scary-looking.

Motivation and Context

Obsoletes #2888.

How Has This Been Tested?

Ran the unit tests, did a couple of manual smoke runs.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@escapedcat
Copy link
Member

Thanks @Zirak !
This is not a breakign change, right? Just to make sure.
Any idea why the checks are not running? We had this issue before when contributors didn't have a circle-ci account. Do you have one?

@Zirak
Copy link
Contributor Author

Zirak commented Jun 17, 2022

Thanks @Zirak !

No no, thank you.

This is not a breakign change, right? Just to make sure.

I was unsure whether to tick the bugfix or breaking change. Naively, this is not breaking; it only deals with the configuration checking mechanism, nothing big.

But it might, maybe? It may be possible to have a configuration which previously passed validation, but no longer now that ajv is more strict.

It's also possible that I did not create a good real-world schema for the rules object:

{
"type": "object",
"required": ["rules"],
"properties": {
"rules": {
"type": "object"
}
}

If we want to err on the side of caution it might be better to define this as breaking? What do you think?

Any idea why the checks are not running? We had this issue before when contributors didn't have a circle-ci account. Do you have one?

You are very correct, I was sure I had one on my personal account yet nada. Created one, do you know if it'll get picked up or should I force-push to retrigger?

@escapedcat
Copy link
Member

I was unsure whether to tick the bugfix or breaking change. Naively, this is not breaking; it only deals with the configuration checking mechanism, nothing big.

But it might, maybe? It may be possible to have a configuration which previously passed validation, but no longer now that ajv is more strict.

It's also possible that I did not create a good real-world schema for the rules object:

In these cases I pray to the OSS gods and rub 🔮 3 times and hope that @armano2 might have a look.

If we want to err on the side of caution it might be better to define this as breaking? What do you think?

Might be safer, yes. Ugh. Another major version :D

Any idea why the checks are not running? We had this issue before when contributors didn't have a circle-ci account. Do you have one?

You are very correct, I was sure I had one on my personal account yet nada. Created one, do you know if it'll get picked up or should I force-push to retrigger?

Please try a push and hopefully this will trigger the checks, thanks!

@Zirak
Copy link
Contributor Author

Zirak commented Jun 17, 2022

If we want to err on the side of caution it might be better to define this as breaking? What do you think?

Might be safer, yes. Ugh. Another major version :D

Huzzah! My apologies for resurrecting a dead horse.

Any idea why the checks are not running? We had this issue before when contributors didn't have a circle-ci account. Do you have one?

You are very correct, I was sure I had one on my personal account yet nada. Created one, do you know if it'll get picked up or should I force-push to retrigger?

Please try a push and hopefully this will trigger the checks, thanks!

It is done!

@armano2
Copy link
Contributor

armano2 commented Jun 17, 2022

there should no be any impacts for this upgrade, as we are using it for simple validation

most notable change is that schema is compiled to es6 instead of es5

as for breaking changes, they mostly relate to strict mode, and as we are not using it we don't really care.

https://ajv.js.org/v6-to-v8-migration.html

you may also consider using https://ajv.js.org/standalone.html#generating-function-s-using-cli, if we see any performance impact (v8 is slower than v6)

@escapedcat
Copy link
Member

Thanks @armano2 for you feedback!
Happy to go with this as non breaking change. If we notice people complain about speed we can adjust I guess.
What do you think @Zirak ?

@Zirak
Copy link
Contributor Author

Zirak commented Jun 21, 2022

as for breaking changes, they mostly relate to strict mode, and as we are not using it we don't really care.

That's sort of true - some strict mode settings are enabled by default in ajv (or rather, strictSchema is true, alongside several other things which are set to log). Some errors resulting from that are fixed as part of this commit.

We can turn all of them off to minimise potential 3rd-party breakage, which sounds like a good compromise.

@escapedcat
Copy link
Member

We can turn all of them off to minimise potential 3rd-party breakage, which sounds like a good compromise.

Sounds good to me

ajv introduced some breaking changes from v6 to v7, and from v7 to v8. Most can
be found in [their migration guide](https://ajv.js.org/v6-to-v8-migration.html).

This commit addresses the following:

- `ErrorObject.dataPath` was renamed to `ErrorObject.instacePath`
- Wordings of some errors changed (e.g. "should" -> "must")
- Strict mode does not allow a top-level `default` value
- Disable [strict mode](https://ajv.js.org/strict-mode.html)
- The `plugins` schema included an invalid object definition for `rules`
- `schemaId` is no longer an ajv option
- `missingRefs` no longer seems to affect the usage of ajv in the project
- `addKeyword` has its signature changed

The tests pass and everyone's seemingly happy; do say if anything's still
scary-looking.

Obsoletes conventional-changelog#2888.
@escapedcat escapedcat merged commit b0e8917 into conventional-changelog:master Jun 24, 2022
@escapedcat
Copy link
Member

Thanks! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants