boardgameio / boardgame.io

State Management and Multiplayer Networking for Turn-Based Games
https://boardgame.io
MIT License
10.03k stars 709 forks source link

Sync is not exported from internal #681

Closed pociej closed 4 years ago

pociej commented 4 years ago

https://github.com/nicolodavis/boardgame.io/blob/master/packages/internal.ts

I guess :

import { InitializeGame } from '../src/core/initialize';
import { ProcessGameConfig } from '../src/core/game';
import { CreateGameReducer } from '../src/core/reducer';
import { Async } from '../src/server/db/base';

export { Async, ProcessGameConfig, InitializeGame, CreateGameReducer };

should be :

import { InitializeGame } from '../src/core/initialize';
import { ProcessGameConfig } from '../src/core/game';
import { CreateGameReducer } from '../src/core/reducer';
import { Async,Sync } from '../src/server/db/base';

export { Async, Sync, ProcessGameConfig, InitializeGame, CreateGameReducer };
delucis commented 4 years ago

Looks good! Would you like to open a PR?

delucis commented 4 years ago

I think it’s actually already possible to access this as:

import { StorageAPI } from 'boardgame.io';

class MyStorage extends StorageAPI.Sync {}

But it would be good to expose Sync and Async in the same way.

pociej commented 4 years ago

https://github.com/nicolodavis/boardgame.io/pull/682 PR created

pociej commented 4 years ago

@delucis it seems that its worse. StorageAPI is not exported at all from the package, try to run tests in your firebase connector.import { StorageAPI } from 'boardgame.io' gives you undefined. In same file i linked above we should have

...
import * as StorageAPI from '../src/server/db/base' 
....
export { StorageAPI, Async, Sync, ProcessGameConfig, InitializeGame, CreateGameReducer };

I would even remove redundant Sync and Async export but if im not blind, now everything storage related is broken.

delucis commented 4 years ago

Are you writing Typescript or JavaScript? The import { StorageAPI } from 'boardgame.io' is Typescript specific, so that might be the problem? All tests pass for the firebase connector and I have used it in production.

pociej commented 4 years ago

Im writing Typescript, and while i can import anything i want from boardgame.io, StorageAPI is undefined. Where it is exported? because 1) It is undefined for me 2) i went through whole code base and i didnt see any export that gives access to StorageAPI 3) Same undefined i got when i cloned your firebase package and run the tests. My hyphothesis is that you have some old installation of the package that exported things properly and something is broken in one of latest versions. Is that possible?

delucis commented 4 years ago

The export is here and aliased via the “types” field in package.json:

https://github.com/nicolodavis/boardgame.io/blob/f83c4f6c5618fce437dddba3c42158914b6686fb/src/types.ts#L10

These were released in v0.39.3 and as far as I can tell, are still available, so not sure exactly why you’re seeing undefined. Could you provide more debugging info (platform, global package installs, package versions listed by npm etc.)?

Can you successfully import other types? For example:

import { Game } from 'boardgame.io'
pociej commented 4 years ago

Yes @delucis im able to import other types. To make sure it is not related to my project config i created one file npm package that contains two lines of code

import { StorageAPI } from "boardgame.io";
const async = StorageAPI.Async;

in file index.ts.

First strange thing is that after tsc index.ts and node index.js i had to manuall install react. Anyways, after intalled it stopped screaming for react but instead throws error that Cannot read property Async of undefined. Other types are imported without problems, and all of them, including StorageAPI are visible in IntelliSense. Im running on macOs 10.13.6 with node 12.16.3 and typescript 3.8.3. npm list -depth=0 gives only

├── boardgame.io@0.39.10
└── react@16.13.1

React i have to install manually because otherwise it throws missing module error. Probably due to some react related exports from boardgame.io. I dont think it is important.

delucis commented 4 years ago

Thanks — still not sure of the cause but I’m sure we’ll figure it out eventually! 😅

A couple of additional things might help diagnose this:

pociej commented 4 years ago

@delucis Nothing in global

grzegorzs-Mac-mini:bgio-problem grzegorz$ npm list -g --depth=0
/Users/grzegorz/.nvm/versions/node/v12.16.1/lib
└── npm@6.13.4

reproduction repo : https://github.com/pociej/bgio-storageapi-problem

delucis commented 4 years ago

Hi @pociej — thanks for your patience with this and the reproduction.

Banged my head against a few walls, but I do now understand 🙌

You can import { StorageAPI } from 'boardgame.io' and this works in your repo too. BUT (and I should have realised this sooner) these are just the types. So this is useful to type things in an implementation with StorageAPI.CreateGameOpts etc., but doesn’t expose the actual Sync/Async base classes we need (just their types).

To actually use the base classes you have to do import { Async, Sync } from 'boardgame.io/internal', which is what you fixed in #682. Does this make sense? I guess you need a release to be able to use the Sync export or can you use Async for now?

pociej commented 4 years ago

Ech what can i say. Thx a lot @delucis You are right and that should be obvious for me :/ My excuse is that it is my first time with typescript. Anyways, in regards to your question about release, that depends on decision we have to make here : https://github.com/nicolodavis/boardgame.io/issues/679. If as @nicolodavis suggested i can create PR to main repo then its not needed, otherwise yeah, release would be useful.

delucis commented 4 years ago

Wasn’t obvious to me, so no reason it should be obvious to you either! :-D

I agree that a Local Storage implementation could be reasonably maintained within the project as suggested — it’s a simple enough and standard enough API — so we can continue discussion in #679. :+1: