Update to TS5 #1240
Update to TS5 #1240
Conversation
| // ts-expect-error should error because 'measurement' isn't an accepted prop | ||
| // unforutnately it does error. There's something I'm missing here | ||
| // @ts-expect-error should error because 'measurement' isn't an accepted prop | ||
| render(<ProductPrice data={getProduct()} measurement="" />); |
There was a problem hiding this comment.
There was a problem hiding this comment.
From the comment, it seems like the situation was: I was expecting it to error, but it wasn't erroring for some reason. But now with the newest update, it seems TS figured out that it should be erroring (as was expected), so your change seems correct.
If that makes sense? haha. 👍
There was a problem hiding this comment.
// unforutnately it does error. There's something I'm missing here
I probably meant to write
// unforutnately it doesn't error. There's something I'm missing here
There was a problem hiding this comment.
Hah I see, thanks for checking! 🙌
| (AsType extends keyof React.JSX.IntrinsicElements | ||
| ? Omit< | ||
| React.ComponentPropsWithoutRef<AsType>, | ||
| keyof CustomBaseButtonProps<AsType> | ||
| > | ||
| : React.ComponentPropsWithoutRef<AsType>); |
There was a problem hiding this comment.
FYI It looks like the important part here is the actual discrimination. Apparently, TS5 is more strict. The second part (the "else") could be {} (empty object) or even the same Omit<...> thing and would still work.
.changeset/purple-weeks-ring.md
Outdated
| Update to TypeScript 5. If you have `typescript` as a dev dependency in your app, change its version as follows: | ||
|
|
||
| ```diff | ||
| "devDependencies": { | ||
| ... | ||
| - "typescript": "^4.9.5", | ||
| + "typescript": "^5.1.6", |
There was a problem hiding this comment.
Do you know if the type fixes are still compatible with TS 4? Is moving to TS 5 a requirement if we merge?
There was a problem hiding this comment.
Yes, it works in TS4 as well. These are just generic fixes we needed but TS4 was not catching.
For example, see a prop like measurement should throw a TS error but it wasn't doing it before (it does now): https://github.com/Shopify/hydrogen/pull/1240/files#r1297221747
TS5 is not a requirement but, since the CLI installs it for tarnspiling stuff, it's better to update projects and avoid dependency duplication.
There was a problem hiding this comment.
I've reworded the changeset to make this more clear and also explained how to change the TS version in VSCode 👍
| TYPE_NAVIGATE: 1, | ||
| TYPE_RELOAD: 2, | ||
| TYPE_BACK_FORWARD: 3, |
There was a problem hiding this comment.
Don't change these values - These are code mapping for PerformaneNavigation api
https://developer.mozilla.org/en-US/docs/Web/API/PerformanceNavigation#instance_properties
There was a problem hiding this comment.
Hmm... this is just a unit test so I guess I can just ts-ignore my way but even that MDN link says it should start with 0, no? 🤔 -- Or am I reading it wrong?
The TS types are also showing 0,1,2 instead of 1,2,3:
There was a problem hiding this comment.
Oh .. I guess I had it wrong this whole time (facepalm)
Closes #1175
Continues #778 and fixes the issues found there.