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

chore: add build target nodejs #2082

Merged

Conversation

fabianbormann
Copy link
Contributor

Dear Mithril-team,

we need a commonjs package to run the mithril-client-wasm module within our ibc gateway setup.

Would it be possible to bundle the module for the nodejs target as well and publishing it within your pipeline to the npm package regsitry? I added the needed line to your Makefile.

Thanks for your support and keep up the great work ! 💪
Fabian

@jpraynaud
Copy link
Member

Hi @fabianbormann, thanks for creating this PR.

It will require some more work to implement a NodeJS compatible npm package for the Mithri client WASM.
I have created the issue #2091 for that 👍

@jpraynaud jpraynaud changed the title chore: add build taget nodejs chore: add build target nodejs Nov 6, 2024
@jpraynaud
Copy link
Member

@fabianbormann can you sign your commits as it is mandatory for merging on this repository?

@fabianbormann
Copy link
Contributor Author

Sure will do

@fabianbormann fabianbormann force-pushed the chore/add_nodejs_target branch from f5ad231 to 0ff64a9 Compare November 6, 2024 21:35
@fabianbormann
Copy link
Contributor Author

@jpraynaud I signed the commit ✅

@jpraynaud
Copy link
Member

Hi @fabianbormann, I see that the branch has conflicts and can not be merged as is.
Can you sync your fork and rebase your branch on main?

@fabianbormann
Copy link
Contributor Author

@jpraynaud I'm done with resolving the conflicts 😊

@jpraynaud
Copy link
Member

@jpraynaud I'm done with resolving the conflicts 😊

@fabianbormann I think something is wrong with all these conflicts that should not be happening in the branch. There should be only 2 commits in this branch:

  • 0ff64a9
  • and one that got trashed in which I added a comment explaining the command you added is experimental

I think that you should do the following to get the PR ready to be merged:

  • make a copy of the existing branch add_nodejs_target
  • delete your branch add_nodejs_target
  • sync your fork
  • create a new branch with the same name add_nodejs_target
  • cherry pick the commit 0ff64a9
  • push force the branch add_nodejs_target

@fabianbormann fabianbormann force-pushed the chore/add_nodejs_target branch from 03dda9c to c789b56 Compare November 7, 2024 16:12
@fabianbormann
Copy link
Contributor Author

I don't know what went wrong, but after throwing the branch away and adding the line again it looks better now

main...fabianbormann:mithril:chore/add_nodejs_target

@jpraynaud
Copy link
Member

I don't know what went wrong, but after throwing the branch away and adding the line again it looks better now

main...fabianbormann:mithril:chore/add_nodejs_target

All good now 🎉 Once the CI run is green, I will merge the PR.
Thanks for the contribution @fabianbormann!

@jpraynaud jpraynaud merged commit 8db2941 into input-output-hk:main Nov 7, 2024
45 checks passed
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.

5 participants