Skip to content
This repository has been archived by the owner on Oct 5, 2023. It is now read-only.

feat: rewriting the library in typescript #210

Merged
merged 5 commits into from
Aug 10, 2022
Merged

Conversation

erunion
Copy link
Member

@erunion erunion commented Aug 10, 2022

🧰 Changes

I've rewritten this library in TypeScript. I also pulled in our jsdoc standards from @readme/eslint-config so all methods in the library now have more accessible docs (along with the new TS types that are available).

That's it!

@erunion erunion added the enhancement New feature or request label Aug 10, 2022
@@ -12815,17 +12815,3 @@ Object {
],
}
`;

exports[`#validate should colorize errors when \`opts.colorizeErrors\` is present 1`] = `
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This got removed because we're skipping this test and Jest didn't carry it over when it moved the snapshot. If we ever re-enable this test this snapshot will come back then.

} else if (typeof obj === 'object') {
return 'json';
} else if (typeof obj === 'string') {
if (obj.match(/\s*{/)) {

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data

This [regular expression](1) that depends on [library input](2) may run slow on strings with many repetitions of ' '.
@erunion erunion requested a review from kanadgupta August 10, 2022 04:32
Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of tiny nits/questions but otherwise looking good!

.npmignore Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
__tests__/index.test.ts Show resolved Hide resolved
README.md Show resolved Hide resolved
erunion and others added 2 commits August 10, 2022 08:36
@erunion erunion requested a review from kanadgupta August 10, 2022 15:39
Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nit, I just realized we run the tsc build twice in CI. Once here:

- run: npm run build

And once here, in the test script:

"test": "tsc; jest --coverage"

Will defer to you on which one to keep!

Copy link
Member

@kanadgupta kanadgupta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise LGTM 🚀

@erunion
Copy link
Member Author

erunion commented Aug 10, 2022

Removed the tsc from the test step. We have that in oas and I think it's an early artifact of our ts-jest usage that we don't actually need anymore.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants