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: Join type #1427

Merged
merged 3 commits into from
Mar 8, 2024
Merged

fix: Join type #1427

merged 3 commits into from
Mar 8, 2024

Conversation

zewa666
Copy link
Contributor

@zewa666 zewa666 commented Mar 8, 2024

fixes #1422

Copy link

stackblitz bot commented Mar 8, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@zewa666
Copy link
Contributor Author

zewa666 commented Mar 8, 2024

I think this should do it. I couldn't really reproduce your original issue but, removing the // @ts-ignore reveleaded it as well for me. So now that one is also gone.

I've also made sure to add some tests if we need to test that further. So far my custom type assertions were enough but we might should go with something like tsd if we want to do more proper checks.

@zewa666
Copy link
Contributor Author

zewa666 commented Mar 8, 2024

ahh ok now I got ya. It doesn't fail in the IDE but when running pnpm run bundle. ok need to check more on that

@ghiscoding
Copy link
Owner

ahh ok now I got ya. It doesn't fail in the IDE but when running pnpm run bundle. ok need to check more on that

hmm actually it failed when I ran the full Build and after going into the file, I did see the error in the IDE/VSCode as shown in the issue print screen. Also FYI, the reason this came up was that I deleted the pnpm lock file and deleted node_modules, because I wanted get rid of the ip package security issue (when a lock file exists, it sometime doesn't pick up version patch fixes).

Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.8%. Comparing base (373c312) to head (c657092).
Report is 1 commits behind head on master.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1427   +/-   ##
======================================
  Coverage    99.8%   99.8%           
======================================
  Files         199     199           
  Lines       21611   21611           
  Branches     7229    7229           
======================================
  Hits        21547   21547           
+ Misses         64      58    -6     
- Partials        0       6    +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zewa666
Copy link
Contributor Author

zewa666 commented Mar 8, 2024

ok rm -rf node_modules might was the missing thing for me. Anyways, good the bundler caught it as well

@ghiscoding
Copy link
Owner

ok rm -rf node_modules might was the missing thing for me. Anyways, good the bundler caught it as well

I'm a bit surprised, I thought that the lock file was enough to do the update, however I'm not even sure since when this error actually existed. I thought it was the latest TypeScript 5.4 that was released a couple days but as it turns out, the previous version also failed on this type... anyway, as long as we fixed it for the future :)

BTW, I saw Cypress failing again so I restarted it

@zewa666
Copy link
Contributor Author

zewa666 commented Mar 8, 2024

well if you can't repro it locally and the bundler cant either plus the original intent still is in place ... I'd say we're good until the next time ;)

@ghiscoding ghiscoding merged commit 21c76cc into ghiscoding:master Mar 8, 2024
6 checks passed
@ghiscoding
Copy link
Owner

Thanks a lot for this PR, you even added more unit tests which is always very welcome... again thx for all your contributions :)

BTW, have you had a chance to try the Dark Mode yet? 😉

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.

TypeScript type error cannot assign unknown to string...
2 participants