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: Fix client types & move BoardProps to React package #752

Merged
merged 12 commits into from
Jul 18, 2020

Conversation

delucis
Copy link
Member

@delucis delucis commented Jun 26, 2020

Fixes #751 (regressions introduced in #744).

  1. Users using strict typechecking would see errors when importing the plain JS client because import Debug from './debug' would point to a JS file that isn’t included in dist/types/src. I’ve restructured the debug type declaration, moving it to debug/index.ts file alongside debug/index.js, which re-exports the Svelte component. This outputs an appropriate type declaration and appears to fix errors in test builds. This PR uses the ambient type declaration for *.svelte files that Svelte provides to import the Debug component directly again. Svelte is moved to be a direct dependency so Svelte types are available to boardgame.io users.

  2. Exporting BoardProps (for use in React board components) from the top-level package breaks strict typechecking for non-React users, because they won’t have installed @types/react, @types/prop-types etc. Instead I’ve moved this to the React package:

    import { Client, BoardProps } from 'boardgame.io/react'
    import { G } from './types';
    
    const TicTacToeBoard = (props: BoardProps<G>) => null
  3. Because we can now import Svelte components in Typescript, this also removes the exception of the debug package not being defined as a .ts file.

delucis added 2 commits June 21, 2020 12:01
BoardProps depends on some types from `@types/react`, which won’t be 
installed for TS users not using React, so exporting it from the 
top-level `'boardgame.io'` module causes errors for those users.
This places the Svelte Debug component type definition in an index.ts 
alongside the index.js that re-exports the Svelte component.
@delucis delucis requested a review from nicolodavis June 26, 2020 10:50
@delucis
Copy link
Member Author

delucis commented Jun 26, 2020

@nicolodavis Tests are failing pretty catastrophically, but I can’t see what’s different between the changes here and the successful test runs on the main branch. Any pointers?

@delucis
Copy link
Member Author

delucis commented Jun 30, 2020

FWIW, I found this: babel/babel#11427 (comment)

This suggests updating Babel to ^7.9.0 for compatibility with newer Node versions (and to fix this error). Should I give that a go?

@nicolodavis
Copy link
Member

nicolodavis commented Jun 30, 2020

@delucis OK, let's try that.

Sorry I haven't had a chance to look at this yet.

@delucis
Copy link
Member Author

delucis commented Jul 2, 2020

@nicolodavis Did you say you had been using a working Svelte & Typescript combo for Boardgame Lab?

I suspect the slightly hacky patches I’ve been chasing here to get Svelte and the TS client to talk nice aren’t going to work and wonder if there’s a better tooling solution (preprocessing Svelte to TSX before it hits the Typescript compiler maybe?)

I’ve tried reading up about this, but TS support seems pretty new to Svelte and I’m finding it hard to find reliable solutions (most seem to be focused on using TS inside your <script> in Svelte files).

@nicolodavis
Copy link
Member

nicolodavis commented Jul 4, 2020

On Boardgame Lab I'm using Svelte components and importing TypeScript files inside them.

I think we're dealing with the reverse here (trying to import a Svelte component into a TypeScript file). Your intuition is correct that we should be processing Svelte before the TypeScript compiler sees it. Does reversing the order in which the Svelte and TypeScript processors are defined in the Rollup config fix this?

@delucis
Copy link
Member Author

delucis commented Jul 6, 2020

Does reversing the order in which the Svelte and TypeScript processors are defined in the Rollup config fix this?

That doesn’t seem to be sufficient — although it’s possible I’m just not quite getting everything to click. I’ve opened sveltejs/language-tools#288 to see if there’s a standard solution.

Edit: No luck there, posted to StackOverflow to see if anyone has a good answer.

@delucis
Copy link
Member Author

delucis commented Jul 10, 2020

@nicolodavis OK — thanks to some feedback from Rich Harris over on StackOverflow — I think I have this working. Svelte needs to be a direct dependency, so that its type declarations are available for TS users. Then, we import Svelte in types.ts, thereby hoisting its ambient module declaration for *.svelte. This means we can import Svelte files directly in Typescript files and it “just works”™.

@nicolodavis
Copy link
Member

Nice work @delucis!

@nicolodavis nicolodavis merged commit 271aecd into master Jul 18, 2020
@nicolodavis nicolodavis deleted the delucis/fix/client-types branch July 18, 2020 02:48
gvillette pushed a commit to gvillette/boardgame.io that referenced this pull request Jul 19, 2020
…#752)

* refactor(packages): Expose BoardProps type via 'boardgame.io/react'

BoardProps depends on some types from `@types/react`, which won’t be 
installed for TS users not using React, so exporting it from the 
top-level `'boardgame.io'` module causes errors for those users.

* refactor(client): Structure debug types to output a .d.ts file

This places the Svelte Debug component type definition in an index.ts 
alongside the index.js that re-exports the Svelte component.

* Revert "refactor(client): Structure debug types to output a .d.ts file"

This reverts commit e8f0735.

* refactor(client): Add svelte types to tsconfig, typing `.svelte` files

* refactor(packages): Convert debug package to TS

* feat(types): Hoist ambient  declaration in main types file

* chore(deps): Make Svelte a dependency

For TS users, Svelte needs to be installed so its ambient types can be 
used.

* build(tsconfig): Remove `types` field

Now the Svelte types are imported in `src/types.ts`, we don’t need to 
tell `tsc` about them in `tsconfig`.
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.

Bug to build on release 0.39.14
2 participants