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

build: update dev deps #240

Merged
merged 3 commits into from
Aug 4, 2024
Merged

build: update dev deps #240

merged 3 commits into from
Aug 4, 2024

Conversation

RebeccaStevens
Copy link
Collaborator

fix #236

It looks like a lot but most of it is just lining updates.
Look at just the first commit to see what's actually changed.

@RebeccaStevens
Copy link
Collaborator Author

Looks like the testing update I made doesn't work in node 16.
How would you like to resolve this? Drop support for node 16? or should I see if I can get it working.

@vitaly-t
Copy link
Owner

vitaly-t commented Aug 4, 2024

Looks like the testing update I made doesn't work in node 16. How would you like to resolve this? Drop support for node 16? or should I see if I can get it working.

What's the minimum version that does work? is it v18? Technically, this is just for testing and assembling a release, it is not a functional limitation, so I am quite ok to up min node version to 18, if needed ;)

@vitaly-t
Copy link
Owner

vitaly-t commented Aug 4, 2024

Other than that, as I went through the scope of changes - that looks huge! I'm particularly intrigued by the addition of type everywhere. Can you tell me, please, why it became needed?

@RebeccaStevens
Copy link
Collaborator Author

Looks like the testing update I made doesn't work in node 16. How would you like to resolve this? Drop support for node 16? or should I see if I can get it working.

What's the minimum version that does work? is it v18? Technically, this is just for testing and assembling a release, it is not a functional limitation, so I am quite ok to up min node version to 18, if needed ;)

Looks like v18 works

Copy link
Owner

@vitaly-t vitaly-t left a comment

Choose a reason for hiding this comment

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

Couple things - comments in JSON, plus the Node version.

tsconfig.base.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@RebeccaStevens
Copy link
Collaborator Author

addition of type everywhere. Can you tell me, please, why it became needed?

First thing to note, I replaced ts-node with tsx.
When running the tests, tsx will strip out all the imports/exports marked as type imports/exports. If they weren't stripped out we'd get a runtime error when trying to import the type at runtime.
All though it's possible to not have to explicitly mark each type import, and continue to run things as we did with ts-node, explicitly marking them comes with additional improvements.
Additionally, I setup the the eslint rule @typescript-eslint/consistent-type-imports and '@typescript-eslint/no-import-type-side-effects to enforce this (they have fixers so you don't need to worry about explicitly adding type when writing code).

@vitaly-t
Copy link
Owner

vitaly-t commented Aug 4, 2024

addition of type everywhere. Can you tell me, please, why it became needed?

First thing to note, I replaced ts-node with tsx. When running the tests, tsx will strip out all the imports/exports marked as type imports/exports. If they weren't stripped out we'd get a runtime error when trying to import the type at runtime. All though it's possible to not have to explicitly mark each type import, and continue to run things as we did with ts-node, explicitly marking them comes with additional improvements. Additionally, I setup the the eslint rule @typescript-eslint/consistent-type-imports and '@typescript-eslint/no-import-type-side-effects to enforce this (they have fixers so you don't need to worry about explicitly adding type when writing code).

Thank you for the clarification! 🤗

@vitaly-t
Copy link
Owner

vitaly-t commented Aug 4, 2024

@RebeccaStevens

LGTM, thank you for this incredible work, I would not have been able to make it myself!

Leaving it for you to merge whenever you think it is ready 😉

@RebeccaStevens RebeccaStevens merged commit 78ec0b6 into main Aug 4, 2024
7 checks passed
@RebeccaStevens RebeccaStevens deleted the deps-update branch August 4, 2024 13:13
@vitaly-t vitaly-t mentioned this pull request Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update all DEV dependencies
2 participants