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

fix: minimal ts erros for group-ui imports #79

Merged
merged 3 commits into from
Jul 13, 2023
Merged

Conversation

ryanchristo
Copy link
Member

@ryanchristo ryanchristo commented Jul 5, 2023

This pull request includes minimal typescript fixes needed for the amino signing support solution in groups-ui:

This is only a temporary fix and generating the code will overwrite the changes here but this would unblock us from adding amino signing support to the groups-ui.

@ryanchristo ryanchristo deleted the ryan/ts-errors branch July 12, 2023 21:24
@ryanchristo ryanchristo restored the ryan/ts-errors branch July 12, 2023 21:54
@ryanchristo ryanchristo reopened this Jul 12, 2023
@ryanchristo ryanchristo marked this pull request as ready for review July 12, 2023 22:57
@ryanchristo
Copy link
Member Author

This is not the ideal solution but I think we should tag another alpha version with the changes here so that we can at least get the groups ui working with amino support in the meantime while continuing to investigate a solution to #84.

@ryanchristo ryanchristo requested a review from a team July 12, 2023 23:01
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

I pulled this locally, then run yarn build and then using yarn link to use my local version of @regen-network/api in groups-ui ryan/10-amino-signing-support but I still get typescript errors in dev or build...
Maybe I have something wrong in my set up or I've missed a step, I haven't dug too much into it at this point.

Other than that I'm fine with those changes so we can move forward with that, as long as we add a note in the README about that, so pre-approving.

@ryanchristo
Copy link
Member Author

Maybe I have something wrong in my set up or I've missed a step, I haven't dug too much into it at this point.

Hmm... I'm not sure. I was importing the local package in package.json to test. I haven't used yarn link before. I remove the node modules and rebuild with the local import to test. Probably not the most efficient but it does the trick. I just double checked to make sure and build and dev in groups-ui seems to be working for me.

If you run yarn build, you will overwrite the changes here. Maybe that's what happened?

Other than that I'm fine with those changes so we can move forward with that, as long as we add a note in the README about that, so pre-approving.

Note added 28a68b2

@ryanchristo ryanchristo merged commit 204a208 into main Jul 13, 2023
@ryanchristo ryanchristo deleted the ryan/ts-errors branch July 13, 2023 16:28
@ryanchristo
Copy link
Member Author

ryanchristo commented Jul 13, 2023

Maybe I have something wrong in my set up or I've missed a step, I haven't dug too much into it at this point.

It might also have been strict mode being set within groups ui (see regen-network/groups-ui@29e58c4). I was using this locally when testing as well and had not committed to the groups pull request yet.

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.

2 participants