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

API changes #28

Closed
bpierre opened this issue Mar 8, 2020 · 6 comments · Fixed by #42
Closed

API changes #28

bpierre opened this issue Mar 8, 2020 · 6 comments · Fixed by #42

Comments

@bpierre
Copy link
Contributor

bpierre commented Mar 8, 2020

Issues with the current API

  1. In a normal scenario, app authors have to try / catch the activate() and store the error in the state so that users can be presented a connection error message. This is verbose and takes us farther from the goal of keeping the boilerplate code as minimal as possible useWallet().
  2. Passing both the connector identifier and the state, using activating, activated and connected, requires consumers of the library to test several variables to know the current state and the connector identifier.
  3. Using a mix of the term “activated” and the term “connected” is confusing. “activated” is coming from EIP 1102, which both defines and deprecates it. Most Ethereum providers use the term “connect” for this (see image below).

Proposed changes

The goal of these changes is generally to:

  • Make it possible to use the library by only importing the default export, useWallet().
  • Remove the confusion between “connected” and “activated” by only using the term “connect”.
  • Keep the boilerplate to a minimum for simple cases.

connect() and disconnect()

connect(id) and disconnect() would replace activate(id) and deactivate(). This is to remove any reference to the term “activation” and increase consistency.

function App() {
  const { account, connect, disconnect } = useWallet()
  return (
    <button onClick={account ? disconnect : connect}>
      {account ? 'Connect' : 'Disconnect'}
    </button>
  )
}

status

status would be a new exported value that would contain the current status of the wallet connection. It would have take one of these values at any given time:

  • "disconnected": no wallet connected (default state).
  • "connecting": trying to connect to the wallet.
  • "connected": connected to the wallet (i.e. the account is available).
  • "error": a connection error happened.

These are strings and not symbols so that users don’t have to import many symbols.

These values would get deprecated: activated, activating and connected.

Having a status connected guarantees that account is not null.

function App() {
  const { account, connect, status } = useWallet()

  if (status === 'connecting') {
    return <p>Connecting…</p>
  }

  if (status === 'connected') {
    return <p>Connected as {account}</p>
  }

  // status === 'disconnected' or status === 'error'
  return <button onClick={() => connect('injected')}>Connect</button>
}

error and reset()

error is a new value that contains the error object that comes with an "error" status.

There are different types of errors exported by module:

  • UnsupportedChainError
  • UnsupportedConnectorError
  • RejectedActivationError
  • ConnectorConfigError

error would be null when the status is not error, otherwise it would get populated by the corresponding error instance.

Users would have the possibility to import the error constructors and use instanceof, or to use the name property.

The message property can also be used to display the current error.

Calling reset() will make status go back to disconnected, and error back to null. Calling connect() would also clear the error state before initiating the new connection.

function App() {
  const { account, connect, error, reset, status } = useWallet()

  if (status === 'connecting') {
    return <p>Connecting…</p>
  }

  if (status === 'connected') {
    return <p>Connected as {account}</p>
  }

  if (status === 'error') {
    return (
      <div>
        <p>Error: {error.message}.</p>
        <button onClick={() => reset()}>OK</button>
      </div>
    )
  }

  // status === 'disconnected'
  return <button onClick={() => connect('injected')}>Connect</button>
}

@Evalir @sohkai Let me know what you guys think!

@Evalir
Copy link
Contributor

Evalir commented Mar 10, 2020

Overall I really like this! This really makes the API leaner and overall simpler to use, especially due to the status property which I like a lot. I have a few words on this:

Calling reset() will make status go back to disconnected, and error back to null. Calling connect() would also clear the error state before initiating the new connection.

What's the difference between reset and disconnect? At first glance, it doesn't appear too clear to me. I could just do reset every time, or, I could do disconnect. Are there any caveats such as more state changes on the former? If so, maybe it could be more clear on what the difference is, or even merge both to be only one way of disconnecting from the wallet to be more opinionated.

Another thing is that when I read reset, I imagined a function that both disconnected and connected me back to the wallet with just pressing a button. Maybe we can choose another name for this, or, further clarify what this does compared to disconnect.

Just my two gwei! Let me know what you think :) I really like the new API proposal.

@bpierre
Copy link
Contributor Author

bpierre commented Mar 11, 2020

@Evalir Excellent point!

I thought of it this way:

reset() is only here to reset the internal state introduced by error. A non-null error can only happen in a disconnected state. Following this idea, calling it while we are connected would do nothing, same as calling it when disconnected but without error.

disconnect() implies that we are connected without error. Following the idea of using it to disconnect only, calling it while being disconnected would do nothing.

Now, I think you are absolutely right: the role of these two is not obvious, and they could be combined into a single function since disconnecting implies clearing the error and vice versa. Let’s do this!

We could go for the name disconnect(), but I think it would be a bit odd to call it while not being disconnected, only to clear the error. Using reset() to disconnect would not be as explicit as disconnect(), but I think it works when explained as a “general reset of the provider state”. We could also make sure it is well explained in the documentation.

So I think we could go for reset() and deprecate disconnect(). What do you think?

@Evalir
Copy link
Contributor

Evalir commented Mar 11, 2020

Hmmm, In retrospect, It might be good to have both reset() and disconnect(); we just need to make very clear what each one is doing. If we were to deprecate one over the other, just to make things consistent I'd keep disconnect(). 😃

@bpierre
Copy link
Contributor Author

bpierre commented Mar 11, 2020

haha interesting 😄 I still think we could try to have them as a single function rather than two, because they both will probably start by calling the other anyway. What made you change your mind?

About disconnect(), I think it works really well for the action of disconnecting (obviously), but not so much to clear the error. Don’t you think it would be a bit counter intuitive to clear the error using disconnect(), even though we are not connected?

In the case of reset(), I think it can be explained as “completely reset the state of useWallet()”, i.e. disconnected and without errors. It might not be as obvious for someone thinking “how do I disconnect?”, but the documentation could help making that clear.

We could also think of alternatives to express that the entire state is going to be set to its default (I’m trying to avoid using the word “reset” here 😆).

@Evalir
Copy link
Contributor

Evalir commented Mar 12, 2020

Haha! It's not that I changed my mind much, it's just that maybe we can make those two functions coexist, but we may need a better name for reset (naming is hard!).

Regarding disconnect(), I actually didn't feel so weird about clearing errors; for me when you disconnect, it feels like the library doesn't know about your "wallet" anymore; it just doesn't have a reason to hold onto information. However, using disconnect() for just clearing errors is not something ideal 😅, in that case, I really think reset(), properly explained and renamed, makes sense.

@sohkai
Copy link
Contributor

sohkai commented Mar 17, 2020

Really nice changes, and agree this will make the API much smoother to use! With reset() available, you can probably drive an entire account connection / disconnection flow via just the exposed state from useWallet() ❤️!


So I think we could go for reset() and deprecate disconnect(). What do you think?

I agree with this; I presume this would also clear any localstorage cache about the last connected wallet / etc (which is also what we would want to do on disconnect anyway).

There are different types of errors exported by module:

A bit of bikeshedding on the error naming (assuming we get to choose these names):

UnsupportedChainError => ChainUnsupportedError
UnsupportedConnectorError => ConnectorUnsupportedError
RejectedActivationError => ConnectionRejectedError
ConnectorConfigError => ConnectorConfigError

@bpierre bpierre pinned this issue Apr 17, 2020
@bpierre bpierre unpinned this issue Jul 27, 2020
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 a pull request may close this issue.

3 participants