APIDevTools / json-schema-ref-parser

Parse, Resolve, and Dereference JSON Schema $ref pointers in Node and browsers
https://apitools.dev/json-schema-ref-parser
MIT License
952 stars 227 forks source link

Breaking Change: Rewrite repo in typescript, change tests to vitest, fix small bugs #300

Closed jonluca closed 1 year ago

jonluca commented 1 year ago

I rewrote the repo in typescript and changed the testing scheme to use vitest. I also bumped some dependencies and removed others.

This is a pretty major refactor but all tests still pass.

I also undid the type: module and made the required node version the stable one (node 16) instead of 17.

Important differences:

wparad commented 1 year ago

I feel like this is going to be impossible to review. How do you expect the review to happen with: image

jonluca commented 1 year ago

I've done this with @philsturgeon for a few repos in the OpenAPI space. I'm also comfortable taking over as maintainer given https://github.com/APIDevTools/json-schema-ref-parser/issues/285

philsturgeon commented 1 year ago

From a user perspective is this is breaking change or is this a huge tech debt cleanup? We just released v10 so if its not a BC for the user then v10.1 would be good, but a v11 would be welcome if this is legitimately a breaking change.

Also semantic release is all screwy so please make sure thats working before v11 or v12 is out.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4045129466


Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/resolvers/file.ts 38 40 95.0%
lib/parsers/binary.ts 36 39 92.31%
lib/parsers/text.ts 43 46 93.48%
lib/index.ts 409 413 99.03%
lib/refs.ts 234 238 98.32%
lib/util/next.ts 7 13 53.85%
lib/pointer.ts 287 296 96.96%
lib/ref.ts 274 288 95.14%
lib/resolvers/http.ts 118 136 86.76%
<!-- Total: 2374 2437 97.41% -->
Totals Coverage Status
Change from base Build 3984690083: 7.9%
Covered Lines: 3180
Relevant Lines: 3263

💛 - Coveralls
jonluca commented 1 year ago

@philsturgeon I think this should be a breaking change. While all tests pass this is a significant enough refactor I'd feel most comfortable with a major version bump.

It also changes things like the required engine and dependency versions, and there might be weirdness in some peoples systems. I've tested it as thoroughly as I can think, but it doesn't feel like a 0.x bump

coffeebe4code commented 1 year ago

Am I understanding that this bumped the repo to 9.1.1 as well?

We have a breaking change in our code. We have had to go back to 9.0.9. 9.1.0 would probably be fine, but we no longer wanted to receive updates with ~ so we are going back to 9.0.9.

Has this project moved to new maintainers?

philsturgeon commented 1 year ago

@coffeebe4code some changes from main snuck into 9.1.1 when I had to manually tag it due to semantic release going wonkier than I've ever known it to go. 9.1.2 solved the problem. That's not related to v10 or this PR.

@jonluca fair enough, go for it. v11!

@jamesmoschou tagging you in on this one.

/fades away into the nearest bush.

jonluca commented 1 year ago

@jamesmoschou can you please take a look? Want to make sure this works + gets out before I work on the next set of PRs

jcmosc commented 1 year ago

@jonluca what's the benefit of moving to yarn?

I'm worried it will introduce another dependency for people. Whereas npm comes with node out of the box and I think has gotten a lot better since yarn originally came about.

jonluca commented 1 year ago

No real reason I suppose, I havent revisited npm since moving to yarn a few years ago and its my default for new projects. Happy to move back to npm if that's an issue though

jcmosc commented 1 year ago

I only went through the lib files and had a few questions. I'm assuming the changes to .spec.ts files are just ts/prettier changes?

Reading up a bit more, I think yarn is fine as it just affects contributors, npm install should still work.

Soo happy to get rid of Karma, it was an absolute pain.

jcmosc commented 1 year ago

Also I'm wondering if we should sort out https://github.com/APIDevTools/json-schema-ref-parser/issues/303 first?

jonluca commented 1 year ago

Yes the spec changes are:

Contents of the tests are identical, with one added test

And regarding #303 - I think this was an issue with moving to an esm + cjs build. This rewrite should work out of the box

Here is a sample project with this new version where that example works properly.

capture 2023-01-30 at 10 01 32 AM
jcmosc commented 1 year ago

Thanks for clarifying my questions, happy to merge.

Do we want to next version to be 10.1 or 11.0? I think you and @philsturgeon were discussing a major version bump? If so, I think semantic-release is going to deploy 10.1 as-is without a BREAKING CHANGE: commit.

jonluca commented 1 year ago

If we squash the commit won't it use the title of the PR? I could be mistaken, I'll push an update tomorrow morning if not

jcmosc commented 1 year ago

Ahh, squashing will work. But it's more of a GitHub feature to use the PR name than the semantic-release feature https://semantic-release.gitbook.io/semantic-release/support/troubleshooting#squashed-commits-are-ignored-by-semantic-release

approving!

github-actions[bot] commented 1 year ago

:tada: This PR is included in version 10.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

philsturgeon commented 1 year ago

Well shit, it was v10.1 anyway. LMK if I gotta nuke it from orbit.

jonluca commented 1 year ago

Ha I was wrong, no it's ok, we can keep it as is. I'll be making a few more PRs in the coming days so we can just move forward then