-
Notifications
You must be signed in to change notification settings - Fork 709
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
TypeScript integration #190
Comments
I'm open to adding typescript definition files if you want to send me a PR. I would prefer that the codebase stays in JS for now, though. |
@shirakaba if you'd like a starting point– @felizardo has a file in his turnato repo: There was also a little discussion around TS when discussing the Angular client: #131 |
@chrisheninger That's invaluable, thank you! Would a single-file declaration describing the whole codebase via many |
For now, I've had to do declaration files alongside each JS file because I'm unsure how else to make test files for the typings. The test files are similarly littering the UI folder. I've also been incrementally adding the types into one global declaration file so that we have both options. You can see my progress on my fork, on the shirakaba/typescript branch. I've so far typed all the src in the UI folder, but still have a few issues to resolve... The foremost issue is that I don't know how to correctly type Logo and Card (which both use stateless component functions – what's more, they're assigned to const variables, and Logo adds in a default export to increase the headache further). Some help there would be appreciated.
There's plenty more work to be done, still. Would welcome any extra pairs of hands. |
I figured it out with a little reference to https://github.com/piotrwitek/react-redux-typescript-guide#component-typing-patterns and some guesswork. I now have Should each of these UI elements accept all props specified for their element type, e.g.: |
Typings for |
@shirakaba Is the intent that these typings ship with boardgame.io instead of in DefinitelyTyped? I see that they're in the repo now in your fork, but wasn't sure if that was a temporary thing as you worked on the typings. |
@scally I'd ideally prefer to ship them with boardgame.io directly, so that maintainers can keep the JS typings definitely in step for each given version of the repo. Otherwise, it becomes unclear which version of the typings one would need to install to match, say, v1.2.3 of the repository (a common headache for TS devs). More than anything though, I'm not sure how to concatenate individual typings into a single file; there are a few things like default exports and modules that I don't know how to handle in the form of a single-file declaration – which would also impede me from launching it as a DefinitelyTyped project. I'm not sure what the linting settings for DefinitelyTyped are, either – likely I'd have to rewrite a bunch of stuff. Still open for discussion about how best to organise the files (ideally, they should only end up being seen in |
I've just finished making typings for the |
I've now written typings for all the JS in On reflection, I've just noticed that I could write typings for the four CSS files, too 🤔 could try, I guess. Blockers:
|
Another option that would eliminate most of the blockers would be to keep it in a separate repo for now until we reach v1.0 and have a stable API (and merge it in then). I worry that the lack of TypeScript expertise and the ever changing API during the alpha stage would make development inconvenient for developers in this repo. |
@nicolodavis Sorry for the late reply.
For now, I'm happy to fill up a separate repo with TypeScript examples and refining the types. It's looking like the path of least resistance may end up being creating a single-file global declaration after all (as the JS is packaged rather than left as individual source files), so we may even have the option of going through DefinitelyTyped in early stages before merging in post-alpha, although that's rather unattractive for me (the typing and linting strictness is of insanely high standard for DefinitelyTyped, and I have dozens of files to get in line).
I'll admit I can see refactoring the JS becoming harder if types need to be kept in step each time. |
Thanks @shirakaba! Yeah, I do think this will be the easiest way to make progress going forward until we hit v1.0 (I'll probably upload a roadmap for this so people have a general sense of the timeline). In the meantime, once you're satisfied with your typings, let me know so I can link to your repo from the documentation here. |
I've now combined all the individual-file declarations into one whole-repo https://github.com/shirakaba/boardgame.io/tree/shirakaba/typescript-examples
However, I'm getting the classic packaging cache problem: facebook/react-native#4968 and so can't check whether the typings actually work when distributed as one single file. @felizardo Did you experience any similar problems when setting up the TicTacToe example? Does your example still compile if you swap your Edit: I've tried making a whole new project, separate from the repo, but that's facing the same error, too :( I'm blocked now. |
@shirakaba Just in case the DefinitelyTyped option could save you some hassle, I researched some of the pain points you mentioned.
Some type definitions include a version at the top indicating which release it matches, like this:
It looks like it might be defined per-package like this: Also, tslint (at least its extension in VSCode) can autofix a lot of problems without complaint on save. |
Just out of curiousity @nicolodavis is there a particular thing keeping you from wanting to use Typescript for this project? It seems like it might be the path of least resistance to integrating types here. |
One thing to add/clarify: I can't get a single-file |
Thus, DefinitelyTyped could be considered if the js source were to be distributed as individual files rather than bundles. |
For me, it's just a lack of knowledge of Typescript that wants me to keep it separate for now. That and the fact that I think the somewhat fluid API at the moment will be harder to modify with more things to keep in sync. I'm totally open to getting it in after we reach v1.0. Which is also a reminder for me to get started on documenting the roadmap to v1.0. EDIT: Roadmap link |
@nicolodavis Thanks for that roadmap just now. |
Oh, I am also maintaining my own .d.ts for my project, lets see if we can collaborate for a single .d.ts :) |
@flamecoals My declarations were written a year ago, and I didn't maintain them further, so I'd recommend you just continue with your own. There were some issues with my typings in any case:
I personally think that the way forward is to migrate the codebase to TS (this would only take a weekend) and distribute it as a library of modules, leaving the bundling to the consumer (there is no way to auto-generate declarations for a bundle of TS modules). However, Nicolo is only open to a migration upon reaching a stable version. And I will admit that TS is a double-edged sword when it comes to Redux-based projects; the Intellisense is great, but the typings can be hard to set up. I see that there's a Gitter. I'll hang out there and get re-acquainted with this project :) |
Thanks. I will do my best to make this reusable by others, even if we need to change over time. I will see if there is any way to add automated tests to see if the types broke. |
I'm more open to TypeScript these days having gained a bit more familiarity with it. I'm not opposed to converting some of the code in If either of you has some time, I'm happy to look at a PR that adds some types to |
Oh, cool :)
@nicolodavis This is a valid concern, although the other side of the coin is that it may attract TypeScript devs (and there are quite a lot of us in the React Native community). I've always held back from contributing because I feel a bit powerless to help if typings aren't in place to make clear all the contracts. I'd much rather the IDE enforce my typings than have to hope that the documentation is up-to-date and sufficient.
Incremental adoption is certainly possible, but presents some problems:
Incremental adoption does make sense in terms of reviewing PRs and reducing merge conflicts from active PRs. I'd love to try, if we can clarify the issues of bundling and shipping. |
Right now Rollup generates a few different files for various subpackages (one for each file in the package directory). Regardless of whether we convert some (or all) the code to TS, we'll still need to generate these JS files in the final NPM as well as a minified bundle that users can insert via a I'm thinking that we can just convert all the code that corresponds to one subpackage to TS as a pilot and see how it goes.
|
I see, so that's the use case of the bundle. Okay, we'll need to research solutions for offering typings post-bundling.
Can do. Example TS library (disregarding bundling for now)
To answer your two questions, here's how I prepare my own TS libraries (
Benefits of writing source in TS
I'm saying that the
If we write everything in TS, we'd have two options:
In practice, TS libraries always go with option 2 (as I've illustrated in my tree structure). Distributing declarations for a bundleThe moment we produce a bundle from all our transpiled JS sources, our typings no longer match the way that the developer will be importing the modules. So either we need to write such typings or find a plugin for the bundler to do it for us automatically. This may actually be easier nowadays than I've historically thought. Here's some quick research: Providing definitions for the bundleManualSource: https://stackoverflow.com/a/41016522/5951226
|
Option 2 sounds good. That would keep our existing NPM package relatively unchanged, except that each JS file now has a About the bundle, I'm OK if we don't ship any typings for it. We'd then force TypeScript devs to import our modules (rather than the bundle, which is only used by JS devs that want a quick prototype without Webpack). This is a reasonable constraint to place on TS devs, who probably have more sophisticated build environments to start out with anyway. |
So, just to make sure we're on the same page, our next step is to convert the sources that feed into They'll still be able to import the other modules as type |
thanks @jasonharrison it seems that the PR you're linking only allowed the in term of typings, it won't help much with our TS projects yet hopefully types are going to pop faster now that you've started the TS integration, good job 👍 |
@Oliboy50 Yup, my PR was so BGIO can begin to use TypeScript :) I actually am not sure how to tell NPM about our declaration files (blah.d.ts)... If you're familiar and you have the time, maybe you could help me with this? |
This is more or less done now. The only thing that remains is to export (tsc generated) type definitions in the NPM and then users will have types for the external interfaces of boardgame.io. |
@nicolodavis looking forward to it! |
This is available now — if not documented yet. General types can be imported directly from boardgame.io, for example: import type { Game } from 'boardgame.io';
interface G {
score?: number;
deck: string[];
}
const game: Game<G> = {
name: 'my-game',
// setup’s return will be checked against your G type
setup: () => ({
deck: [],
}),
} |
How exactly are types being added? This is what setup?: (ctx: Ctx, setupData?: any) => any; but in dist/types/src/types.d.ts it shows up as simply setup?: Function; Which is not nearly as helpful |
@crhistianramirez Which version are you using? This was fixed in 866e897 and released in v0.39.4. |
@delucis that is the version I have downloaded. I downloaded zip of the release and inspected the contents. Not seeing it there yet. Thinking it might not have been built before it was published |
Ah, that ZIP is just the source code, not the built release, but taking a look, the release does seem to be missing this update, so maybe your hunch is correct: https://unpkg.com/browse/[email protected]/dist/types/src/types.d.ts @nicolodavis Is it possible Typescript did an incremental build when you released and didn’t include these updates or that the TS command isn’t auto-run on release? |
The TS command is run by Rollup, so I'm not sure what's broken here. Let me push another release and see if that resolves it. |
@crhistianramirez Can you try |
Looks good, Nicolo! https://unpkg.com/browse/[email protected]/dist/types/src/types.d.ts |
Also, do you guys think that |
Perhaps? It’s actually the same interface as that returned by the |
Looks good!! Yes, I agree |
The new typescript types in I am having some issues with |
@will-hart Yes, the server module is bundled slightly different from the others as you saw discussed in #576, so its types aren’t exposed correctly when doing |
@will-hart This should be fixed once #622 is merged and released. |
@coyotte508 I think this is because the client source code hasn’t been converted to Typescript yet. I’m working on some changes to those files at the moment, so I’ll try to make that conversion while I do. |
Ok :) In the meantime |
#622 is released in |
I was running into some issues using the Specifically, I tried to define my game like this: export const GameDefinition: Game<GameState, Ctx & PlayerPlugin<PlayerState>, CustomSetupData> = {
// game definition elided
} This definition causes my However, the import { PlayerPlugin } from 'boardgame.io/dist/types/src/plugins/plugin-player'; Have I made an error, or is this something that can be fixed? IMO it would make more sense to include |
While we still need better documentation of usage with TypeScript, current releases of boardgame.io are bundled with fairly complete typings, so I believe this can be closed. |
I don't see any TypeScript definition files, nor an
@types/boardgame.io
module onnpm
.The text was updated successfully, but these errors were encountered: