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

Support Node.js #65

Merged
merged 17 commits into from
Feb 2, 2023
Merged

Support Node.js #65

merged 17 commits into from
Feb 2, 2023

Conversation

ferrell-code
Copy link
Contributor

@ferrell-code ferrell-code commented Jan 19, 2023

Support node

todo: Add documentation for using manta.js in node

Simple docs added to README

Signed-off-by: Charles Ferrell <charlie@manta.network>
Signed-off-by: Charles Ferrell <charlie@manta.network>
@ferrell-code ferrell-code changed the title Adds typing to transpiled javascript Support Node.js Jan 20, 2023
Signed-off-by: Charles Ferrell <charlie@manta.network>
@ferrell-code ferrell-code marked this pull request as draft January 20, 2023 04:25
Signed-off-by: Charles Ferrell <charlie@manta.network>
@ferrell-code ferrell-code marked this pull request as ready for review January 23, 2023 17:01
Signed-off-by: Charles Ferrell <charlie@manta.network>
Signed-off-by: Charles Ferrell <charlie@manta.network>
Signed-off-by: Charles Ferrell <charlie@manta.network>
Signed-off-by: Charles Ferrell <charlie@manta.network>
@ferrell-code ferrell-code added C-enhancement Category: An issue proposing an enhancement or a PR with one L-changed Changelog: add these changes to the `changed` section of the changelog A-compatibility Area: Issues and PRs related to Compatibility and removed L-changed Changelog: add these changes to the `changed` section of the changelog labels Jan 25, 2023
ghzlatarev
ghzlatarev previously approved these changes Jan 25, 2023
Copy link
Contributor

@Kevingislason Kevingislason left a comment

Choose a reason for hiding this comment

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

Nice work, I tried to figure this stuff out and it was a nightmare.

After thinking it over, I think we may want to publish both the browser and node.js versions of the code together as a single npm package, assuming that doesn't break things. Users of node.js would simply import from manta.js/node, and users of manta.js would import from manta.js/browser. Running yarn build would run scripts to build for both targets. Since this would break current import paths, we would want to bump the major version of this package as well.

I think this change would help avoid human error while publishing these packages to npm, which could be potentially very serious. It's not strictly correct to have one package.json file for two different packages, because a package can only have one name, and that's what npm looks at when publishing. Although I would prefer to publish these packages in CI, we currently publish packages manually, for better or worse. Whenever I publish manta.js given the configuration in this PR, I would have to manually update the name in the package.json, and be very careful not to confuse the two different build targets. If the process for publishing packages isn't really straightforward there's room for human error, and I've definitely made mistakes in the past.

@ghzlatarev
Copy link
Contributor

Nice work, I tried to figure this stuff out and it was a nightmare.

After thinking it over, I think we may want to publish both the browser and node.js versions of the code together as a single npm package, assuming that doesn't break things. Users of node.js would simply import from manta.js/node, and users of manta.js would import from manta.js/browser. Running yarn build would run scripts to build for both targets. Since this would break current import paths, we would want to bump the major version of this package as well.

I think this change would help avoid human error while publishing these packages to npm, which could be potentially very serious. It's not strictly correct to have one package.json file for two different packages, because a package can only have one name, and that's what npm looks at when publishing. Although I would prefer to publish these packages in CI, we currently publish packages manually, for better or worse. Whenever I publish manta.js given the configuration in this PR, I would have to manually update the name in the package.json, and be very careful not to confuse the two different build targets. If the process for publishing packages isn't really straightforward there's room for human error, and I've definitely made mistakes in the past.

@ferrell-code

@ferrell-code
Copy link
Contributor Author

Yeah I'm looking into the best way to do that

Signed-off-by: Charles Ferrell <charlie@manta.network>
Signed-off-by: Charles Ferrell <charlie@manta.network>
Signed-off-by: Charles Ferrell <charlie@manta.network>
@ferrell-code
Copy link
Contributor Author

ferrell-code commented Jan 31, 2023

Ok this should make it so manta.js defaults to browser build. Please double check this doesn't break anything. It works in both node and browser environment for me.

No changes to browser... I could make it more explicit exporting browser build to ./browser instead of ./, but this way it isn't a breaking change

@ghzlatarev
Copy link
Contributor

Ok this should make it so manta.js defaults to browser build. Please double check this doesn't break anything. It works in both node and browser environment for me.

No changes to browser... I could make it more explicit exporting browser build to ./browser instead of ./, but this way it isn't a breaking change

So still build it with yarn build-node right ?

@ferrell-code
Copy link
Contributor Author

ferrell-code commented Jan 31, 2023

Yeah or there is now a build-all to build both and what ultimately should be run when publishing npm package.

Also you will need to delete your dist directory just once to not corrupt the path scheme

Updated so script is extra paranoid to clean dist directory

Signed-off-by: Charles Ferrell <charlie@manta.network>
@ferrell-code
Copy link
Contributor Author

Also needed to add a few more specs to node specific README. Not sure how easy to follow along it is

@ghzlatarev
Copy link
Contributor

ghzlatarev commented Jan 31, 2023

Yeah or there is now a build-all to build both and what ultimately should be run when publishing npm package.
Also you will need to delete your dist directory just once to not corrupt the path scheme
Updated so script is extra paranoid to clean dist directory

Getting this again after building with yarn build-all and package.json line: "manta.js": "file:../../Manta-Network/sdk/manta-js/package",

TSError: ⨯ Unable to compile TypeScript:
rpc_full_sync_test.ts:2:58 - error TS2307: Cannot find module 'manta.js/node' or its corresponding type declarations.

2 import { MantaPrivateWallet, Environment, Network } from 'manta.js/node';
                                                           ~~~~~~~~~~~~~~~

EDIT: nvm I see what I did wrong. It does work.

@ghzlatarev
Copy link
Contributor

Also needed to add a few more specs to node specific README. Not sure how easy to follow along it is

Also needed to add a few more specs to node specific README. Not sure how easy to follow along it is

Yeah we need to mention it here https://github.com/Manta-Network/sdk/blob/main/manta-js/README.md?plain=1#L13-L18

Signed-off-by: Charles Ferrell <charlie@manta.network>
ghzlatarev
ghzlatarev previously approved these changes Jan 31, 2023
Signed-off-by: Charles Ferrell <charlie@manta.network>
ghzlatarev
ghzlatarev previously approved these changes Jan 31, 2023
Copy link
Contributor

@Kevingislason Kevingislason left a comment

Choose a reason for hiding this comment

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

tested on front end, looks good

Copy link
Contributor

@Kevingislason Kevingislason left a comment

Choose a reason for hiding this comment

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

err sorry, one more thing, still need to bump the version. I think 0.1.0 makes sense since there is significant new functionality but changes are not breaking

Signed-off-by: Charles Ferrell <charlie@manta.network>
ghzlatarev
ghzlatarev previously approved these changes Feb 1, 2023
Kevingislason
Kevingislason previously approved these changes Feb 1, 2023
@Kevingislason
Copy link
Contributor

Just lmk and I can publish the package

@ghzlatarev
Copy link
Contributor

Just lmk and I can publish the package

Merge and publish

@ferrell-code
Copy link
Contributor Author

good on my end, we can bundle #71 in this new release too, or have it bump another version

@Kevingislason
Copy link
Contributor

I'm gonna bump the version to 1.0.0, I think there was a previously published version 0.1.0, and npm won't let me publish over it

Signed-off-by: Kevin Gislason <33131270+Kevingislason@users.noreply.github.com>
@Kevingislason Kevingislason dismissed stale reviews from ghzlatarev and themself via 0928ee4 February 2, 2023 05:26
@Kevingislason Kevingislason merged commit b9271ea into main Feb 2, 2023
@Kevingislason Kevingislason deleted the fer-add-typing branch February 2, 2023 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compatibility Area: Issues and PRs related to Compatibility C-enhancement Category: An issue proposing an enhancement or a PR with one
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants