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

jonluca · 2023-01-22T17:19:01Z

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:

Vitest now uses the builtin c8 coverage tool
The special characters test does not currently work due to a bug in vites filesystem parsing, seen here Tests with special characters in the filename are not processed correctly vitest-dev/vitest#2731 - this shouldn't impact the actual logic of the app, and will work outside of vite
I've changed browser compatibility to use jsdom environment in node instead of using karma. This gets us a little less reliability in exchange for a lot more speed in testing. Also, with modern browser envs, I feel like needing the entirety of karma is overkill
I've added prettier and some more eslint rules.
I've changed the default package manager to yarn

wparad · 2023-01-22T20:03:08Z

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

jonluca · 2023-01-22T20:06:17Z

I've done this with @philsturgeon for a few repos in the OpenAPI space. I'm also comfortable taking over as maintainer given #285

philsturgeon · 2023-01-23T08:31:31Z

package.json

@@ -37,67 +41,68 @@
  },
  "license": "MIT",
  "funding": "https://github.com/sponsors/philsturgeon",
-  "main": "lib/index.js",
-  "typings": "lib/index.d.ts",
-  "exports": {


Will this provide the same ES modules + CJS support that was added in #295? I don't want to annoy older users, but perhaps if we've raised the node version high enough then thats moot now anyway?

Yes, I moved this back to being a cjs package. I also changed the pinned version from node experimental (17) to LTS (16). 14 will EOL in April so we should still support 16.

philsturgeon · 2023-01-23T08:35:56Z

package.json

    "@jsdevtools/host-environment": "^2.1.2",
-    "@jsdevtools/karma-config": "^3.1.7",


Thank you, this is helping me deprecate those older @jsdevtools packages that were mainly used for APIDevTools packages. https://www.npmjs.com/package/@jsdevtools/karma-config

I can remove the host-env one too, since the environment is always running in node.

philsturgeon · 2023-01-23T08:42:14Z

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.

…x package versions

coveralls · 2023-01-23T16:37:15Z

Pull Request Test Coverage Report for Build 4045129466

2374 of 2437 (97.41%) changed or added relevant lines in 21 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+7.9%) to 96.225%

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%

Totals
Change from base Build 3984690083:	7.9%
Covered Lines:	3180
Relevant Lines:	3263

💛 - Coveralls

jonluca · 2023-01-23T16:41:10Z

@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 · 2023-01-23T18:42:23Z

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 · 2023-01-25T21:39:58Z

@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 · 2023-01-29T18:58:23Z

@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 · 2023-01-30T01:31:27Z

@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 · 2023-01-30T01:39:01Z

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 · 2023-01-30T02:08:59Z

lib/index.ts

+   * @returns - The returned promise resolves with the parsed JSON schema object.
+   */
+  public parse(schema: RefParserSchema): Promise<JSONSchema>;
+  public parse(schema: RefParserSchema, callback: SchemaCallback): Promise<void>;


I've not done method overloading in TypeScript before. Do they all have to have the same return value, or should the variants with a callback return void instead of Promise?

Well the types that are currently used in the repo are actually incorrect - all the function calls return a Promise, since it's an async function. This just types it correctly.

However, with this type-definition overloading, it's saying that when you pass in just schema, the promise will resolve to a JSONSchema, whereas if you pass in a callback the callback will be called and the promise will be void.

I don't really like the usage of arguments in all these functions, and it would be nice to be able to write new APIs that actually do satisfy some of these apis. They're written in a very "accepting" way right now, IMO.

jcmosc · 2023-01-30T02:33:23Z

lib/ref.ts

+   * @returns
+   */
+  static is$Ref(value: any): value is { $ref: string; length?: number } {
+    return value && typeof value === "object" && typeof value.$ref === "string";


This used to have an extra value.$ref.length > 0 check. Do we still need it?

I dont think so but I'll add it back

jcmosc · 2023-01-30T02:40:13Z

lib/refs.ts

+   * @param [options]
+   * @returns - Returns the resolved value
+   */
+  get(path: string, options?: $RefParserOptions): JSONSchema4Type | JSONSchema6Type | JSONSchema7Type {


The return value here might not be JSON Schema. If this library is used on an OpenAPI document for example, it could be any one of these https://spec.openapis.org/oas/v3.1.0#components-object.

I believe that's covered by these subtypes - this isn't JSONSchema4, it's JSONSchema4Type, which is a few primitives and is pretty much just a catch all for any valid json schema object

ahh you're right, sorry.

jcmosc · 2023-01-30T03:31:50Z

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 · 2023-01-30T03:33:00Z

Also I'm wondering if we should sort out #303 first?

jonluca · 2023-01-30T15:00:58Z

Yes the spec changes are:

Prettier
Slight changes due to vitest differences
Typescript additions

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.

jcmosc · 2023-02-01T04:52:12Z

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 · 2023-02-01T06:49:59Z

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 · 2023-02-01T22:49:38Z

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 · 2023-02-01T22:55:36Z

🎉 This PR is included in version 10.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

philsturgeon · 2023-02-01T22:58:44Z

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

jonluca · 2023-02-03T17:31:25Z

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

jonluca force-pushed the typescript branch 2 times, most recently from 7641ffb to e5a4d56 Compare January 22, 2023 18:07

philsturgeon reviewed Jan 23, 2023

View reviewed changes

jonluca force-pushed the typescript branch 5 times, most recently from afe3610 to 37ec1dc Compare January 23, 2023 16:28

feat: Rewrite repo in typescript, change testing schema to vitest, fi…

f2130ef

…x package versions

jonluca force-pushed the typescript branch from 37ec1dc to f2130ef Compare January 23, 2023 16:33

coffeebe4code mentioned this pull request Jan 23, 2023

ES Modules #295

Merged

philsturgeon requested a review from jcmosc January 25, 2023 21:37

philsturgeon mentioned this pull request Jan 25, 2023

Taking over, and ESMification readmeio/json-schema-ref-parser#64

Closed

jonluca enabled auto-merge (squash) January 29, 2023 18:57

jcmosc reviewed Jan 30, 2023

View reviewed changes

fix: re-add length check, add gitignore for type

b8d3fc8

jcmosc approved these changes Feb 1, 2023

View reviewed changes

jonluca merged commit a5b3946 into APIDevTools:main Feb 1, 2023

github-actions bot added the released label Feb 1, 2023

mgabeler-lee-6rs mentioned this pull request Feb 6, 2023

Many missing exported types since #300 #307

Closed

kumv-net mentioned this pull request Jan 23, 2024

Behavior change for relative input path in 0.26.0 ferdikoomen/openapi-typescript-codegen#1970

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jonluca commented Jan 22, 2023 •

edited

Loading

wparad commented Jan 22, 2023

jonluca commented Jan 22, 2023

philsturgeon Jan 23, 2023

jonluca Jan 23, 2023

philsturgeon Jan 23, 2023

jonluca Jan 23, 2023

philsturgeon commented Jan 23, 2023

coveralls commented Jan 23, 2023 •

edited

Loading

jonluca commented Jan 23, 2023

coffeebe4code commented Jan 23, 2023

philsturgeon commented Jan 25, 2023

jonluca commented Jan 29, 2023

jcmosc commented Jan 30, 2023

jonluca commented Jan 30, 2023

jcmosc Jan 30, 2023

jonluca Jan 30, 2023

jcmosc Jan 30, 2023

jonluca Jan 30, 2023

jcmosc Jan 30, 2023 •

edited

Loading

jonluca Jan 30, 2023

jcmosc Feb 1, 2023

jcmosc commented Jan 30, 2023

jcmosc commented Jan 30, 2023

jonluca commented Jan 30, 2023 •

edited

Loading

jcmosc commented Feb 1, 2023

jonluca commented Feb 1, 2023

jcmosc commented Feb 1, 2023

github-actions bot commented Feb 1, 2023

philsturgeon commented Feb 1, 2023

jonluca commented Feb 3, 2023

		"@jsdevtools/host-environment": "^2.1.2",
		"@jsdevtools/karma-config": "^3.1.7",

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

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

Conversation

jonluca commented Jan 22, 2023 • edited Loading

wparad commented Jan 22, 2023

jonluca commented Jan 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philsturgeon commented Jan 23, 2023

coveralls commented Jan 23, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4045129466

💛 - Coveralls

jonluca commented Jan 23, 2023

coffeebe4code commented Jan 23, 2023

philsturgeon commented Jan 25, 2023

jonluca commented Jan 29, 2023

jcmosc commented Jan 30, 2023

jonluca commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcmosc Jan 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcmosc commented Jan 30, 2023

jcmosc commented Jan 30, 2023

jonluca commented Jan 30, 2023 • edited Loading

jcmosc commented Feb 1, 2023

jonluca commented Feb 1, 2023

jcmosc commented Feb 1, 2023

github-actions bot commented Feb 1, 2023

philsturgeon commented Feb 1, 2023

jonluca commented Feb 3, 2023

jonluca commented Jan 22, 2023 •

edited

Loading

coveralls commented Jan 23, 2023 •

edited

Loading

jcmosc Jan 30, 2023 •

edited

Loading

jonluca commented Jan 30, 2023 •

edited

Loading