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

Import hs-client into the project. #796

Merged
merged 3 commits into from
May 30, 2023

Conversation

nodech
Copy link
Contributor

@nodech nodech commented Jan 16, 2023

hsd and hs-client are tightly coupled. When major or minor changes occur hs-client needs to reflect them right away. The versioning scheme should be exactly the same as well, which has not been the case up til now (we used patches there). If versions don't match, users will have trouble figuring out which hs-client version to use with which hsd.

The most troublesome part is the major breaking changes, if something breaks hs-client it becomes troublesome to manage hs-client. Having client alongside with hsd make this really simple.

Standalone version of hs-client is also necessary, for those who want minimal functionality w/o all heavy dependencies in their projects. So this PR introduces script in scripts/gen-hsclient.js which can aid with pushing and publishing of the hs-client both to github and npm.

To sum it up:

  • Pros
    • Easier to work with, especially for breaking API changes.
    • Easier and better management version of the hs-client.
  • Cons
    • Contributors need to be aware of the hs-client files and not introduce dependencies that make standalone 'hs-client' heavy (see scripts/gen-hsclient.js).
    • Extra step in the release process (which should have already been happening if we want better versioning of hs-client)

You can review usage update here: https://github.com/nodech/hsd/blob/6bd7cc0a825facf9dec7894888950939d7b427df/docs/release-files.md#hs-client

@nodech nodech requested review from pinheadmz, chjj and rithvikvibhu and removed request for chjj January 16, 2023 19:07
@coveralls
Copy link

coveralls commented Jan 16, 2023

Coverage Status

Coverage: 68.39% (-0.05%) from 68.438% when pulling 2dc2c48 on nodech:import-hs-client into 6314c1a on handshake-org:master.

@nodech nodech added pkg part of the codebase wallet-api part of the codebase node-api part of the codebase labels Jan 17, 2023
@nodech nodech added this to the hsd 6.0.0 milestone Jan 20, 2023
@rithvikvibhu
Copy link
Member

I think this makes a lot of sense and each api change won't need to track a sibling PR on another repo. I've tried this branch and the generated hs-client and other than the slightly awkward process to dev hs-client (discussed in https://t.me/hns_tech/74474) everything looks good to me.

Q: What do you think about the exposed bins of both hsd and hs-client now? Currently, hsd has hs*-cli and hs-client has hs*-cli, hs*-rpc. I get that hs-client will still be a separate package, but maybe hsd can add hs*-rpc as well? So users won't need to install hs-client for the rpc shortcuts. I bring this up because hsd and hs-client both can't be installed globally without --force overwriting the two conflicting bin files.

Non-blocking suggestions:

  • HS_CLIENT_DIR but as an arg to the script
  • an option to the script arg to only generate content, without any git work

@nodech nodech added the breaking-major Backwards incompatible - Release version label Mar 9, 2023
Copy link
Member

@rithvikvibhu rithvikvibhu left a comment

Choose a reason for hiding this comment

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

This is awesome, can finally maintain both packages together. The new script args work great. Left some non-blocking comments.

docs/release-files.md Outdated Show resolved Hide resolved
scripts/gen-hsclient.js Show resolved Hide resolved
scripts/gen-hsclient.js Show resolved Hide resolved
docs/release-files.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-major Backwards incompatible - Release version node-api part of the codebase pkg part of the codebase wallet-api part of the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants