chore(repo,clerk-js): Update TS to v5 & Update emotion minor#1877
chore(repo,clerk-js): Update TS to v5 & Update emotion minor#1877
Conversation
🦋 Changeset detectedLatest commit: caf1f2f The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| elementDescriptor={leftIconElementDescriptor} | ||
| elementId={leftIconElementId} | ||
| icon={leftIcon} | ||
| icon={leftIcon as React.ComponentType} |
There was a problem hiding this comment.
So this is a bit complicated 😆
Due to https://github.com/clerkinc/javascript/blob/caf1f2fd1a8f7b187e152e0e095d01095b41ca98/packages/clerk-js/src/ui/elements/ArrowBlockButton.tsx#L12 this type here is wrong. But you just can't set it to the correct React.ComponentType because the type is referenced in other places (as a type alias).
So yeah, instead of fixing stuff in 10 places I just type-casted 🙈
There was a problem hiding this comment.
What do we need to fix the underlying type issue?
Can we either try to do it now or create a ticket with more details?
There was a problem hiding this comment.
I don't know yet, since this would require some bigger investigation/changes rather than just keeping things as-is for now.
It comes down to us re-using the props here:
Then in other places we use it like this: https://github.com/clerkinc/javascript/blob/8434782c53147e4e3333746fd6096212dfcaa51d/packages/clerk-js/src/ui/components/SignIn/SignInAccountSwitcher.tsx#L51-L57
But the code I changed here only wants ComponentType:
So it's a big problem and fixing this would block me from shipping the more important work
There was a problem hiding this comment.
The type isn't actually wrong here, see the isIconElement boolean on L47 and handling the case where leftIcon is an element on L99. It seems that with the TS upgrade though the type narrowing isn't working as it was before 😞 :
I think @LekoArts's fix is good for now, we could also use @ts-expect-error and leave a comment stating we already have a type guard.
| "outDir": "./dist", | ||
| "declarationDir": "./dist/types" | ||
| "declarationDir": "./dist/types", | ||
| "noImplicitReturns": false |
There was a problem hiding this comment.
We only warn against this in our code (and have some instances where we need to fix this). During compilation then it fails unless turned off. Since we don't enforce it in our normal code I opted for this for now.
brkalow
left a comment
brkalow
left a comment
There was a problem hiding this comment.
Approving! I think we can opt to use @ts-expect-error instead of casting for the type issue, but either approach works.
|
This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
4 participants
Description
So to be able to use the
Bundleroption in https://www.typescriptlang.org/tsconfig#moduleResolution we need TypeScript v5. 5.1 introduced something new for JSX: https://devblogs.microsoft.com/typescript/announcing-typescript-5-1/#decoupled-type-checking-between-jsx-elements-and-jsx-tag-typesTo make TS v5 and emotion work, we need to update emotion a little bit.
Both updates are harmless:
Checklist
npm testruns as expected.npm run buildruns as expected.Type of change
Packages affected
@clerk/clerk-js@clerk/clerk-react@clerk/nextjs@clerk/remix@clerk/types@clerk/themes@clerk/localizations@clerk/clerk-expo@clerk/backend@clerk/clerk-sdk-node@clerk/shared@clerk/fastify@clerk/chrome-extensiongatsby-plugin-clerkbuild/tooling/chore