testground / sdk-js

Other
1 stars 3 forks source link

docs: add types and documentation #11

Closed hacdias closed 3 years ago

hacdias commented 3 years ago
hacdias commented 3 years ago

@jacobheun as I think you have more experience with TypeScript, I'm pinging you here to ask you to validate my TypeScript typings: am I writing them the way they're supposed to be? Is there any configuration that I'm missing? I feel like there's something lacking

jacobheun commented 3 years ago

Tapping @gozala and @hugomrdias for TS typings, they're currently adding typings through js-ipfs/js-libp2p.

We've also started using a github action typechecker, that would be good to setup here as well, https://github.com/gozala/typescript-error-reporter-action.

hugomrdias commented 3 years ago

My advice is to not manually write definitions and do the same thing we are doing in ipfs and libp2p. We are using JSdoc to type the code inline and aegir to check and generate definitions. https://github.com/ipfs/aegir/blob/master/md/ts-jsdoc.md https://github.com/ipfs/js-ipfs/issues/2945

hacdias commented 3 years ago

Thanks @hugomrdias. Quick question, is the bundle size checker tailored only for packages that can go into the browser? For example, here I'm getting the error Can't resolve 'os', which lead me to assume that. I want to make this package work on the browser too in the future, but I'm not sure if I can obtain the network interfaces and the hostname in the browser!

hugomrdias commented 3 years ago

Q: will this be only targeted at node ? we would also might want to include browser, electron, react-native nodes in the tests.

hacdias commented 3 years ago

Q: will this be only targeted at node ? we would also might want to include browser, electron, react-native nodes in the tests.

See comment above. I want to make it work on the browser, but I didn't find alternatives for those functions of the os package...

hugomrdias commented 3 years ago

Thanks @hugomrdias. Quick question, is the bundle size checker tailored only for packages that can go into the browser? For example, here I'm getting the error Can't resolve 'os', which lead me to assume that. I want to make this package work on the browser too in the future, but I'm not sure if I can obtain the network interfaces and the hostname in the browser!

its tailored to everything that might use bundlers and those will not include node globals or builtins

hugomrdias commented 3 years ago

Everything "native" should be injected rather than locking yourself right from the beginning into nodejs

hugomrdias commented 3 years ago

the architecture of this should abstract these things in a way we can implement env specific implementations, and then swap them as needed. so i would recommend thinking a bit about all the info you might need and where that comes from in different envs.

Some solutions might just be injecting as globals or process.env vars or abstract into an interface that can be implemented for different envs. ie. browser doesnt have network interfaces but libp2p in some cases might overcome that or you might need some hooks to orchestrate external nodes to delegate. React native has interfaces but the way to get them is really different from nodejs.

hacdias commented 3 years ago

the architecture of this should abstract these things in a way we can implement env specific implementations, and then swap them as needed. so i would recommend thinking a bit about all the info you might need and where that comes from in different envs.

Some solutions might just be injecting as globals or process.env vars or abstract into an interface that can be implemented for different envs. ie. browser doesnt have network interfaces but libp2p in some cases might overcome that or you might need some hooks to orchestrate external nodes to delegate. React native has interfaces but the way to get them is really different from nodejs.

Sounds reasonable. If we take into account that the test is always started by Testground, we will always have a way to obtain those informations and pass them to the tests somehow. I will rework the environment specific variables so we can inject them. I will probably go for the interface way. Seems more flexible.

hacdias commented 3 years ago

Superseeded by #15

hacdias commented 3 years ago

the architecture of this should abstract these things in a way we can implement env specific implementations, and then swap them as needed. so i would recommend thinking a bit about all the info you might need and where that comes from in different envs.

Some solutions might just be injecting as globals or process.env vars or abstract into an interface that can be implemented for different envs. ie. browser doesnt have network interfaces but libp2p in some cases might overcome that or you might need some hooks to orchestrate external nodes to delegate. React native has interfaces but the way to get them is really different from nodejs.

@hugomrdias After investigating, we can do so for stuff like 'path' and 'os'. However, I can't find any redis client that does not depend on Node built-ins. I will investigate. If you know any, that'd be great.

hugomrdias commented 3 years ago

ah right, the only solution i found for browser connection is to use https://webd.is/ but you can split by env and target browser like and bundlers with an http client to webdis and node with a node native redis-client. if you need help ping me