Migrate playroom internals to TypeScript#313
Conversation
🦋 Changeset detectedLatest commit: 4425d38 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
@michaeltaranto @askoufis breaking down the Vite PR into smaller ones, this migrates files in the |
askoufis
left a comment
askoufis
left a comment
There was a problem hiding this comment.
Thanks for splitting out the PR. Just a few comments.
| "main": "utils/index.ts", | ||
| "types": "utils/index.ts", |
There was a problem hiding this comment.
Do we want to ship typescript as source? The alternative would be a build step. We could just run tsc for now, and graduate to something that offers proper CJS/ESM support (if necessary) later.
There was a problem hiding this comment.
Reflecting on this, maybe for now we could just keep the separate .js and .d.ts files for simplicity, and think about a potential build process later on.
| themes: Record<string, any>; | ||
| components: Array<any>; | ||
| FrameComponent: React.ComponentType<{ | ||
| components: Record<string, ComponentType>; |
There was a problem hiding this comment.
Should this be ComponentType<any> so it will accept any component? Leaving out the generic argument results in it defaulting to {}, so only components that don't take any props would be accepted.
| scope, | ||
| }: { | ||
| code: string | undefined; | ||
| scope: Record<string, ComponentType>; |
There was a problem hiding this comment.
I don't think ComponentType is correct here. You can inject whatever you want with scope, a constant, a function, a component, etc. Maybe Record<string, any> would be more appropriate.
|
@michaeltaranto thanks! I mean I also haven't follow up in like a year so appreciate seeing this making its way into the repo in some way 😄 |
3 participants
Migrates internals to TypeScript. This PR is the first in a series of changes aiming to support Vite. This is the original PR #285, but following Michael suggestion to split up the work.