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.

Sign up for GitHub

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: downstream referential equality errors #144

Merged
merged 4 commits into from
Aug 15, 2022
Merged

fix: downstream referential equality errors #144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
90d2dfa
improve key logic
NoahZinsmeister Aug 12, 2022
689f97e
address comments
NoahZinsmeister Aug 15, 2022
3f29389
fix e2e tests
JFrankfurt Aug 15, 2022
89890f5
style(lint): lint action with ESLint
Aug 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 28 additions & 44 deletions e2e/connect.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,88 +5,72 @@
import '@ethersproject/providers'
import 'jest-environment-hardhat'

import { render, RenderResult, waitFor } from '@testing-library/react'
import { waitFor } from '@testing-library/react'
import { tokens } from '@uniswap/default-token-list'
import React from 'react'

import { SwapWidget } from '../src'
import { renderWidget } from '../src/test'

const HARDHAT_ACCOUNT_DISPLAY_STRING = `${hardhat.account.address?.substring(
0,
6
)}...${hardhat.account.address?.substring(hardhat.account.address.length - 4)}`

describe('connect', () => {
let component: RenderResult
let account: HTMLElement
let connectWallet: HTMLElement
let toolbar: HTMLElement
let tokenSelect: HTMLElement

beforeEach(async () => {
component = render(<SwapWidget tokenList={tokens} />)
connectWallet = await component.findByTestId('connect-wallet')
toolbar = await component.findByTestId('toolbar')
tokenSelect = (await component.findAllByTestId('token-select'))[0]
})

describe('with no params, using fallback JSON RPC URL', () => {
it('prompts for wallet connection in the Wallet and Toolbar', async () => {
expect(toolbar.textContent).toBe('Connecting…')
await waitFor(() => expect(toolbar.textContent).not.toBe('Connecting…'))
expect(connectWallet.textContent).toBe('Connect wallet to swap')
const component = renderWidget(<SwapWidget tokenList={tokens} />)
const toolbar = await component.findByTestId('toolbar')
expect(toolbar.textContent).toBe('Connect wallet to swap')
const connectWallet = await component.findByTestId('connect-wallet')
expect(connectWallet.textContent).toBe('Connect wallet to swap')
})

it('expects widget not to be disabled', async () => {
expect(tokenSelect).toHaveProperty('disabled', true)
await waitFor(() => expect(toolbar.textContent).not.toBe('Connecting…'))
expect(tokenSelect).toHaveProperty('disabled', false)
const component = renderWidget(<SwapWidget tokenList={tokens} />)
const tokenSelect = (await component.findAllByTestId('token-select'))[0]
await waitFor(() => {
expect(tokenSelect).toHaveProperty('disabled', false)
})
})
})

describe('with valid jsonRpcUrlMap', () => {
it('prompts for wallet connection in the Wallet and Toolbar', async () => {
component = render(<SwapWidget tokenList={tokens} jsonRpcUrlMap={{ 1: [hardhat.url] }} />)
expect(connectWallet.textContent).toBe('Connect wallet to swap')
expect(toolbar.textContent).toBe('Connecting…')
await waitFor(() => expect(toolbar.textContent).not.toBe('Connecting…'))
const component = renderWidget(<SwapWidget tokenList={tokens} jsonRpcUrlMap={{ 1: [hardhat.url] }} />)
const connectWallet = await component.findByTestId('connect-wallet')
const toolbar = await component.findByTestId('toolbar')
expect(connectWallet.textContent).toBe('Connect wallet to swap')
expect(toolbar.textContent).toBe('Connect wallet to swap')
})

it('expects widget not to be disabled', async () => {
component = render(<SwapWidget tokenList={tokens} jsonRpcUrlMap={{ 1: [hardhat.url] }} />)
const component = renderWidget(<SwapWidget tokenList={tokens} jsonRpcUrlMap={{ 1: [hardhat.url] }} />)
let tokenSelect = (await component.findAllByTestId('token-select'))[0]
expect(tokenSelect).toHaveProperty('disabled', true)
const toolbar = await component.findByTestId('toolbar')
await waitFor(() => expect(toolbar.textContent).not.toBe('Connecting…'))
expect(tokenSelect).toHaveProperty('disabled', false)
tokenSelect = (await component.findAllByTestId('token-select'))[0]
await waitFor(() => expect(tokenSelect).toHaveProperty('disabled', false))
})
})

describe('with wallet provider', () => {
it('displays connected account chip', async () => {
component = render(<SwapWidget tokenList={tokens} provider={hardhat.provider} />)
const component = renderWidget(<SwapWidget tokenList={tokens} provider={hardhat.provider} />)
const toolbar = await component.findByTestId('toolbar')
await waitFor(() => expect(toolbar.textContent).not.toBe('Connecting…'))
account = await component.findByTestId('account')
expect(account.textContent?.toLowerCase()).toBe(HARDHAT_ACCOUNT_DISPLAY_STRING)
})

it('does not prompt for wallet connection in toolbar', async () => {
component = render(<SwapWidget tokenList={tokens} provider={hardhat.provider} />)
expect(toolbar.textContent).toBe('Connecting…')
await waitFor(
async () => {
toolbar = (await component.findAllByTestId('toolbar'))[1]
expect(toolbar.textContent).toBe('Enter an amount')
},
{ timeout: 10000 }
)
const account = await component.findByTestId('account')
await waitFor(() => expect(account.textContent?.toLowerCase()).toBe(HARDHAT_ACCOUNT_DISPLAY_STRING))
})

it('expects widget not to be disabled', async () => {
component = render(<SwapWidget tokenList={tokens} provider={hardhat.provider} />)
expect(tokenSelect).toHaveProperty('disabled', true)
const component = renderWidget(<SwapWidget tokenList={tokens} provider={hardhat.provider} />)
const toolbar = await component.findByTestId('toolbar')
await waitFor(() => expect(toolbar.textContent).not.toBe('Connecting…'))
expect(tokenSelect).toHaveProperty('disabled', false)
const tokenSelect = (await component.findAllByTestId('token-select'))[0]
await waitFor(() => expect(tokenSelect).toHaveProperty('disabled', false))
})
})
})
28 changes: 23 additions & 5 deletions src/hooks/connectWeb3/useWeb3React.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { Connector, Provider as Eip1193Provider, Web3ReactStore } from '@web3-re
import { WalletConnect } from '@web3-react/walletconnect'
import { SupportedChainId } from 'constants/chains'
import { atom, useAtom } from 'jotai'
import { PropsWithChildren, useEffect, useMemo } from 'react'
import { PropsWithChildren, useEffect, useMemo, useRef } from 'react'
import JsonRpcConnector from 'utils/JsonRpcConnector'

export type Web3Connection = [Connector, Web3ReactHooks]
Copy link
Contributor

Choose a reason for hiding this comment

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

connections is potentially invalid if it changes over time. Does this export need to be reconsidered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, while i'm still not thrilled about this export, i checked and all the other components that use connections are children of Web3ReactProvider, and since the whole point of this pr is ensuring that this component re-renders whenever connections changes, there shouldn't be any immediate issues

my vote would be to merge this and consider a refactor in a later pr? can make an issue if there's agreement

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is the case, could there just be a hook to expose connections within the React lifecycle? I'm fine with doing this as a follow up, but it seems brittle and not future-proof, so I may just do it today.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tracking in #145

Expand Down Expand Up @@ -104,12 +104,30 @@ export function ActiveWeb3Provider({
[jsonRpcUrlMap, defaultChainId]
)

connections = [metaMaskConnection, walletConnectConnectionQR, walletConnectConnectionPopup, networkConnection]
if (integratorConnection) connections = [integratorConnection, ...connections]
const key = useRef(0)
connections = useMemo(() => {
// while react warns against triggering side effects in useMemo,
// in this instance we're only using the mutated value to generate a key,
// so tightly coupling the key update with the memo update shouldn't cause any issues,
// and most clearly expresses the intent
key.current += 1
return [
integratorConnection,
metaMaskConnection,
walletConnectConnectionQR,
walletConnectConnectionPopup,
networkConnection,
].filter((connection): connection is Web3Connection => Boolean(connection))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think you can add the type to the template parameters and just use Boolean, so that the actual code remains simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm i tried this and i don't think it's possible - similar issue (without a fix) described here: microsoft/TypeScript#16069

}, [
integratorConnection,
metaMaskConnection,
walletConnectConnectionQR,
walletConnectConnectionPopup,
networkConnection,
])

const key = `${connections.length}+${Object.entries(jsonRpcUrlMap)}+${propsDefaultChainId}+${defaultChainId}`
return (
<Web3ReactProvider connectors={connections} key={key}>
<Web3ReactProvider connectors={connections} key={key.current}>
{children}
</Web3ReactProvider>
)
Expand Down