Skip to content

migrate to ajv v7 [WIP]#130

Merged
gajus merged 1 commit intogajus:masterfrom
epoberezkin:epoberezkin/ajv-v7
Jan 2, 2021
Merged

migrate to ajv v7 [WIP]#130
gajus merged 1 commit intogajus:masterfrom
epoberezkin:epoberezkin/ajv-v7

Conversation

@epoberezkin
Copy link
Contributor

@epoberezkin epoberezkin commented Dec 6, 2020 •
edited
Loading

@gajus please have a look.

The changes are:

  • switch to ajv v7 - now it compiles both schemas to one file with 2 exports - it reduced the size of compiled code (there is a big shared part) and simplified the code on the call site a bit.
  • refactored schemas to extract shared part. It didn't affect compiled code size - I thought it would - as ajv was already re-using code from duplicated schemas, but I kept it anyway, the schemas are smaller this way - easier to change if needed.
  • js-beautify is no longer included in ajv-pack (there is no ajv-pack too, standalone code generation is included in ajv now), so it is imported directly to format code. It is not necessary, strictly speaking, maybe the compiled code could be just one big string - let me know.

Should it be a major version upgrade? There is no API changes here, but eslint etc. would have 2 different ajv versions in the tree if it's not - not sure if it may cause any conflicts? It is probably worth releasing as minor version and roll-back if there are any unresolvable conflicts.

In any case please hold it until Ajv v7 is released as main version.

@epoberezkin epoberezkin marked this pull request as draft December 6, 2020 14:04
@epoberezkin epoberezkin changed the title migrate to ajv v7 migrate to ajv v7 [WIP] Dec 6, 2020
@coveralls
Copy link

coveralls commented Dec 6, 2020

Pull Request Test Coverage Report for Build 224

  • 2 of 3 (66.67%) changed or added relevant lines in 1 file are covered.
  • 38 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-16.7%) to 56.231%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/validateConfig.js 2 3 66.67%
Files with Coverage Reduction New Missed Lines %
src/validateConfig.js 1 20.0%
src/getBorderCharacters.js 2 0%
src/createStream.js 6 10.2%
src/makeStreamConfig.js 14 10.34%
src/makeConfig.js 15 13.79%
Totals Coverage Status
Change from base Build 220: -16.7%
Covered Lines: 135
Relevant Lines: 236
💛 - Coveralls

@coveralls
Copy link

coveralls commented Dec 6, 2020 •
edited
Loading

Pull Request Test Coverage Report for Build 225

  • 2 of 3 (66.67%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage remained the same at 72.948%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/validateConfig.js 2 3 66.67%
Files with Coverage Reduction New Missed Lines %
src/validateConfig.js 1 30.0%
Totals Coverage Status
Change from base Build 220: 0.0%
Covered Lines: 172
Relevant Lines: 236
💛 - Coveralls

@epoberezkin epoberezkin marked this pull request as ready for review December 6, 2020 16:54
@epoberezkin
Copy link
Contributor Author

epoberezkin commented Dec 6, 2020

Actually the coverage is back where it was, it's probably because the tests failed

@epoberezkin
Copy link
Contributor Author

epoberezkin commented Dec 6, 2020

I did check - eslint seems to be ok with updated table, so probably it should be a minor version.

@gajus gajus merged commit 645060e into gajus:master Jan 2, 2021
@gajus
Copy link
Owner

gajus commented Jan 2, 2021

🎉 This PR is included in version 6.0.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gajus gajus added the released label Jan 2, 2021
@epoberezkin
Copy link
Contributor Author

epoberezkin commented Jan 2, 2021

I've just updated this PR to pull the main version :)

I think server works correctly, but it probably best to update the version anyway so nobody is confused - will submit the new PR.

@epoberezkin
Copy link
Contributor Author

epoberezkin commented Jan 2, 2021

Ah - you've updated already - thank you :)

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

Reviewers

No reviews

Assignees

No one assigned

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@epoberezkin @coveralls @gajus