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

migrate to ajv v7 [WIP] #130

Merged
merged 1 commit into from
Jan 2, 2021
Merged

migrate to ajv v7 [WIP] #130

merged 1 commit into from
Jan 2, 2021

Conversation

epoberezkin
Copy link
Contributor

@epoberezkin epoberezkin commented Dec 6, 2020

@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

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

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

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

@epoberezkin
Copy link
Contributor Author

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

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

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

Successfully merging this pull request may close these issues.

3 participants