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

docs: add types and documentation #11

Closed
wants to merge 6 commits into from
Closed

docs: add types and documentation #11

wants to merge 6 commits into from

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Nov 27, 2020

  • TypeScript typings
  • Update README with some docs

@hacdias
Copy link
Member Author

hacdias commented Dec 4, 2020

@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
Copy link

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
Copy link

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
ipfs/js-ipfs#2945

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
Comment on lines +34 to 35
"lodash.flatten": "^4.4.0",
"winston": "^3.3.3"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be better to use Array.flat and consider pino instead of winston

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The linter complains because Array.flat was not available on Node 10. I could not change the engines to Node 12.

Thanks for the pino suggestion!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you supporting node10 ?

Copy link
Member Author

@hacdias hacdias Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Apparently it's the TS checker. The linter is Ok.

Edit: please ignore this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait why are flattening that method returns an object not a array

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you supporting node10 ?

@hugomrdias every time I change the engines in package.json to:

  "engines": {
    "node": ">=12.0.0"
  },

I get:

./node_modules/.bin/aegir lint-package-json

./package.json
✖ valid-values-engines - node: engines - Invalid value for engines
1 error
0 warnings
Command failed with exit code 2: npmPkgJsonLint /Users/henriquedias/Code/testground/sdk-js/package.json -c /Users/henriquedias/Code/testground/sdk-js/node_modules/aegir/src/config/.npmpackagejsonlintrc.json

Then I saw this and assumed we had to support Node 10.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah got it i will fix that, sorry my bad, that rule is dumb

@hacdias
Copy link
Member Author

hacdias commented Dec 11, 2020

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
Copy link

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

@hacdias
Copy link
Member Author

hacdias commented Dec 11, 2020

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
Copy link

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
Copy link

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

@hugomrdias
Copy link

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
Copy link
Member Author

hacdias commented Dec 11, 2020

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
Copy link
Member Author

hacdias commented Dec 18, 2020

Superseeded by #15

@hacdias hacdias closed this Dec 18, 2020
@hacdias
Copy link
Member Author

hacdias commented Dec 18, 2020

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.

@hacdias hacdias deleted the docs branch December 18, 2020 12:35
@hugomrdias
Copy link

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

@hacdias hacdias mentioned this pull request Jan 8, 2021
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.

None yet

3 participants