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

feat: Angular support for wallet-adapter #42

Merged
merged 32 commits into from
Sep 14, 2021
Merged

Conversation

danmt
Copy link
Contributor

@danmt danmt commented Sep 1, 2021

Overview

As an Angular dev there's no straightforward way of using this package without having to code everything from scratch. This PR intention is to give support for Angular devs trying to integrate @solana/wallet-adapter into their dapps.

Features

In order to ensure this package is analogous to @solana/wallet-adapter-react a list of features is exposed below to make sure nothing is missed.

  • Connect to a wallet
  • Disconnect from a wallet
  • Change selected wallet (disconnecting current wallet and persist in localStorage)
  • Send transaction
  • Sign transaction
  • Sign all transactions
  • Sign message
  • Allow auto connect
  • Listen to wallet adapter events
  • Customizable error handler
  • Mapping from wallet to anchor wallet
  • Managing the connection

NOTES

The state of the wallet and connection is handled by @ngrx/component-store which is the recommended tool, the component-store is very similar to the Providers in the React library. There are some providers that are similar to the Context from React, it just takes a Provider and makes it available for a component and its children.

closes #37

@danmt danmt marked this pull request as ready for review September 1, 2021 14:53
@danmt
Copy link
Contributor Author

danmt commented Sep 1, 2021

CRA depends on babel-loader@8.1.0 and webpack@4.4.0, while Angular depends on babel-loader@8.2.2 and webpack@5.50.0. Angular deps are hoisted to the root by yarn, leading to a warning. I had to do this commit in order to bypass the error.

Might worth taking a second look at it. I'm open to questions as needed!

Copy link
Collaborator

@jordaaash jordaaash left a comment

Choose a reason for hiding this comment

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

Looks really clean, considering I find the Angular/Rxjs stuff absolutely brutal to look at :)

I just have few questions about how we can make sure the features are detectable in TypeScript so devs get compile time errors instead of pushing issues into runtime.

packages/angular/src/wallet/wallet.errors.ts Outdated Show resolved Hide resolved
packages/angular/src/wallet/wallet.store.ts Outdated Show resolved Hide resolved
packages/angular/src/wallet/wallet.store.ts Outdated Show resolved Hide resolved
packages/angular/src/wallet/wallet.store.ts Outdated Show resolved Hide resolved
@jordaaash
Copy link
Collaborator

CRA depends on babel-loader@8.1.0 and webpack@4.4.0, while Angular depends on babel-loader@8.2.2 and webpack@5.50.0. Angular deps are hoisted to the root by yarn, leading to a warning. I had to do this commit in order to bypass the error.

One way to get around this might be to use https://classic.yarnpkg.com/en/docs/selective-version-resolutions/ to force both to resolve to 8.2.x

@dubin-s
Copy link

dubin-s commented Sep 10, 2021

If there is anything I can do to help get this merged in please let me know, would be happy to work with you @danmt to resolve anything.

@danmt
Copy link
Contributor Author

danmt commented Sep 10, 2021

Right on @dubin-s. I'm currently working on my hackathon project so I haven't been able to wrap this one up.

If you want to have early access check this out @danmt/wallet-adapter-angular, it's basically a fork I have for development purposes. I'm using it in a project and it's looking fine. It's literally the same thing you see in the PR plus some minor adjustments.

You can give it a try and drop feedback here, I'd love an extra pair of eyes on it.

@jordaaash
Copy link
Collaborator

Might be good to merge master in, and check the simplifying state changes that have been made to the react package since this is based on that.

@danmt
Copy link
Contributor Author

danmt commented Sep 11, 2021

I can do the sign-methods fix we talked about and the changes you mentioned. I've been using this for a little while and it's looking good, we can push it even if it's not ready to start gathering feedback.

I can have this later today.

@jordaaash
Copy link
Collaborator

Awesome, sounds good. Also, hit me up on Discord -- I'd be happy to take a look at your project if you like.

@danmt
Copy link
Contributor Author

danmt commented Sep 11, 2021

I changed the sign-methods to be Observable<type> | undefined for all sign-methods. Also, made the anchorWallet thing so it returns undefined when the selected wallet cannot sign.

BTW - The only way I could bypass the babel-loader error was by adding SKIP_PREFLIGHT_CHECK=true to every CRA package (example, material-ui-starter, react-ui-starter).

I successfully built every package. It wasn't as easy as I wanted with Lerna but that should work.

@danmt
Copy link
Contributor Author

danmt commented Sep 11, 2021

Once this gets merged and published, will update my example app and will raise another PR for that.

Copy link
Collaborator

@jordaaash jordaaash left a comment

Choose a reason for hiding this comment

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

Thanks for this! Mostly I have a bunch of questions with a few small change requests.

packages/angular/package.json Outdated Show resolved Hide resolved
packages/angular/src/wallet/wallet.store.ts Outdated Show resolved Hide resolved
packages/example/.env Outdated Show resolved Hide resolved
packages/angular/tsconfig.lib.json Outdated Show resolved Hide resolved
packages/angular/src/wallet/wallet.store.ts Outdated Show resolved Hide resolved
packages/angular/src/wallet/wallet.store.ts Show resolved Hide resolved
packages/angular/src/wallet/wallet.store.ts Outdated Show resolved Hide resolved
packages/angular/src/wallet/wallet.store.ts Outdated Show resolved Hide resolved
packages/angular/src/wallet/wallet.store.ts Outdated Show resolved Hide resolved
packages/angular/.prettierignore Outdated Show resolved Hide resolved
@danmt
Copy link
Contributor Author

danmt commented Sep 12, 2021

Yo @jordansexton, I decided to go the extra mile and made some refactoring so the package maps almost 1:1 with the react package. As you would expect, there are some places I couldn't do the exact same thing but I tried to keep the same behavior.

Things to have in mind:

  • I removed _logError which is analogous to onError from the react package. The reason behind it is that I created the error stream I mentioned in our discussion, if the consumer wants to "tap" into the errors being thrown all it takes is to subscribe to the exposed error$.
  • Given that the WalletStore is a class, making the sign methods conditional was a real challenge, I ended up creating a higher-order function per method, each returns undefined if the adapter cannot do that or a function that takes whatever you want to sign and returns an observable that on subscription does the actual signing.
  • I don't have to set up the event listeners and tearing them down because switchMap + the custom operator fromAdapterEvent takes care of that for me.
  • I turned the connect/disconnect methods from effects into methods returning observables, that way the consumers have full control of it. I figured that if it was confusing for both of us, it would be too for a newcomer.

Things left to do:

  • The SKIP_PREFLIGHT thingy, I'm not sure how to deal with it but I'm open to suggestions.

Besides that, I re-ordered values and methods just like the React package for easier follow-up. I have an example app and it works like charm, I feel like once we fix the preflight stuff this is ready to go. I'll raise a PR for the example once this gets merged in.

NOTE: Even though I feel comfortable with it, would love it if you can take a look at it just to make sure I didn't leave any weirdness behind.

@dubin-s
Copy link

dubin-s commented Sep 13, 2021

Going to take a look at this today @danmt. I see there have been lots of updates since I looked at this last. Was MIA over the weekend.

@danmt
Copy link
Contributor Author

danmt commented Sep 13, 2021

Essentially it works the same way, I made adjustments so it was more similar to the react package. As well as adding a bunch of suggestions from @jordansexton. If you're familiar with the react package, this should be easy to follow, if it isn't feel free to drop a comment.

@jordaaash
Copy link
Collaborator

Awesome, thanks @danmt. I'll try to review this today.

* feat: initial commit

* feat: it's alive!!
* chore: bump version

* feat: set up error handler via config

* chore: bump version

* feat: make error handler optional

* feat: prevent error flushing on send transaction

* chore: bump version

* refactor: deprecate the providers

* feat: deprecate connection providers
danmt and others added 2 commits September 14, 2021 09:27
* fix dependency resolution

* fix lint warning

* space

* make error class declarations consistent with base package

* adjust internal method names

* use ReturnType for inference
@jordaaash jordaaash merged commit 2cc614f into anza-xyz:master Sep 14, 2021
@jordaaash
Copy link
Collaborator

Awesome work, thank you @danmt 🚀

@danmt
Copy link
Contributor Author

danmt commented Sep 14, 2021

Wohoo! 🎉

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.

Angular support
3 participants