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

fix(arrayvalidator): fixed runaway type instantiation with TypeScript >=5.1 #275

Merged
merged 3 commits into from
Jun 4, 2023

Conversation

favna
Copy link
Member

@favna favna commented Jun 4, 2023

@codecov-commenter
Copy link

codecov-commenter commented Jun 4, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (06ac013) 99.28% compared to head (0b171c5) 99.28%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #275   +/-   ##
=======================================
  Coverage   99.28%   99.28%           
=======================================
  Files          58       58           
  Lines        3477     3485    +8     
  Branches      713      713           
=======================================
+ Hits         3452     3460    +8     
  Misses         17       17           
  Partials        8        8           
Flag Coverage Δ
16 99.28% <100.00%> (+<0.01%) ⬆️
18 99.28% <100.00%> (+<0.01%) ⬆️
19 99.28% <100.00%> (+<0.01%) ⬆️
20 99.28% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/lib/util-types.ts 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@favna
Copy link
Member Author

favna commented Jun 4, 2023

@sapphiredev pack

@sapphiredev
Copy link

sapphiredev bot commented Jun 4, 2023

Heya @favna, I've started to run the deployment workflow on this PR at 2417255. You can monitor the build here!

@sapphiredev
Copy link

sapphiredev bot commented Jun 4, 2023

Hey @favna, I've released this to NPM. You can install it for testing like so:

npm install @sapphire/shapeshift@pr-275

vladfrangu
vladfrangu previously approved these changes Jun 4, 2023
Copy link
Member

@vladfrangu vladfrangu left a comment

Choose a reason for hiding this comment

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

If it works, merge it

@favna favna merged commit f59d901 into main Jun 4, 2023
@favna favna deleted the fix/typescript-5.1-fix-runaway-type-instantiation branch June 4, 2023 10:56
@SuperchupuDev
Copy link

this fix can probably be reverted now that typescript 5.1.5 was released

@favna
Copy link
Member Author

favna commented Jun 29, 2023

this fix can probably be reverted now that typescript 5.1.5 was released

Fair point but because it's still an issue in older versions of TypeScript and we cannot be guaranteed that everyone uses caret versioning, or that their transitive dependencies are properly updated, we will keep the fix for now. It would be a breaking change if we would want to force a specific TS version.

@SuperchupuDev
Copy link

SuperchupuDev commented Jun 29, 2023

alright but 5.1.3 is the only stable typescript version that had the problem, there are no other versions with the issue, updating to a new patch release shouldn't be that much of an problem? especially to fix a regression. it wouldn't really force people that use older ts versions other than 5.1.3

@imranbarbhuiya
Copy link
Contributor

imranbarbhuiya commented Jun 29, 2023

Why this pr need revert tho? are there any issues with the current approach?

@vladfrangu
Copy link
Member

Unless there are any glaring issues caused by this PR, we won't be reverting it any time soon, as (as far as we know) there are no issues with keeping it. Should that change, you're free to open an issue / revert

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants