Skip to content

Commit

Permalink
Avoid calling setState in callback ref (vercel#18589)
Browse files Browse the repository at this point in the history
  • Loading branch information
Janpot authored Nov 1, 2020
1 parent a6759c6 commit c0ae204
Show file tree
Hide file tree
Showing 4 changed files with 100 additions and 33 deletions.
8 changes: 5 additions & 3 deletions .github/workflows/build_test_deploy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,12 @@ jobs:
steps:
- uses: actions/checkout@v2
- run: git fetch --depth=1 origin +refs/tags/*:refs/tags/*
- run: cat packages/next/package.json | jq '.resolutions.webpack = "^5.0.0-beta.30"' > package.json.tmp && mv package.json.tmp packages/next/package.json
- run: cat packages/next/package.json | jq '.resolutions.react = "^17.0.0-rc.1"' > package.json.tmp && mv package.json.tmp packages/next/package.json
- run: cat packages/next/package.json | jq '.resolutions."react-dom" = "^17.0.0-rc.1"' > package.json.tmp && mv package.json.tmp packages/next/package.json
- run: cat package.json | jq '.resolutions.webpack = "^5.0.0-beta.30"' > package.json.tmp && mv package.json.tmp package.json
- run: cat package.json | jq '.resolutions.react = "^17.0.1"' > package.json.tmp && mv package.json.tmp package.json
- run: cat package.json | jq '.resolutions."react-dom" = "^17.0.1"' > package.json.tmp && mv package.json.tmp package.json
- run: yarn install --check-files
- run: yarn list webpack react react-dom
- run: node run-tests.js test/integration/link-ref/test/index.test.js
- run: node run-tests.js test/integration/production/test/index.test.js
- run: node run-tests.js test/integration/basic/test/index.test.js
- run: node run-tests.js test/integration/async-modules/test/index.test.js
Expand Down
62 changes: 32 additions & 30 deletions packages/next/client/link.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,6 @@ function Link(props: React.PropsWithChildren<LinkProps>) {
}
const p = props.prefetch !== false

const [childElm, setChildElm] = React.useState<Element>()

const router = useRouter()
const pathname = (router && router.pathname) || '/'

Expand All @@ -276,24 +274,6 @@ function Link(props: React.PropsWithChildren<LinkProps>) {
}
}, [pathname, props.href, props.as])

React.useEffect(() => {
if (
p &&
IntersectionObserver &&
childElm &&
childElm.tagName &&
isLocalURL(href)
) {
// Join on an invalid URI character
const isPrefetched = prefetched[href + '%' + as]
if (!isPrefetched) {
return listenToIntersections(childElm, () => {
prefetch(router, href, as)
})
}
}
}, [p, childElm, href, as, router])

let { children, replace, shallow, scroll, locale } = props
// Deprecated. Warning shown by propType check. If the children provided is a string (<Link>example</Link>) we wrap it in an <a> tag
if (typeof children === 'string') {
Expand All @@ -302,22 +282,44 @@ function Link(props: React.PropsWithChildren<LinkProps>) {

// This will return the first child, if multiple are provided it will throw an error
const child: any = Children.only(children)
const childRef: any = child && typeof child === 'object' && child.ref

const cleanup = React.useRef<() => void>()
const setRef = React.useCallback(
(el: Element) => {
// cleanup previous event handlers
if (cleanup.current) {
cleanup.current()
cleanup.current = undefined
}

if (p && IntersectionObserver && el && el.tagName && isLocalURL(href)) {
// Join on an invalid URI character
const isPrefetched = prefetched[href + '%' + as]
if (!isPrefetched) {
cleanup.current = listenToIntersections(el, () => {
prefetch(router, href, as)
})
}
}

if (childRef) {
if (typeof childRef === 'function') childRef(el)
else if (typeof childRef === 'object') {
childRef.current = el
}
}
},
[p, childRef, href, as, router]
)

const childProps: {
onMouseEnter?: React.MouseEventHandler
onClick: React.MouseEventHandler
href?: string
ref?: any
} = {
ref: (el: any) => {
if (el) setChildElm(el)

if (child && typeof child === 'object' && child.ref) {
if (typeof child.ref === 'function') child.ref(el)
else if (typeof child.ref === 'object') {
child.ref.current = el
}
}
},
ref: setRef,
onClick: (e: React.MouseEvent) => {
if (child.props && typeof child.props.onClick === 'function') {
child.props.onClick(e)
Expand Down
50 changes: 50 additions & 0 deletions test/integration/link-ref/pages/click-away-race-condition.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import React, { useCallback, useEffect, useRef, useState } from 'react'
import Link from 'next/link'

const useClickAway = (ref, onClickAway) => {
useEffect(() => {
const handler = (event) => {
const el = ref.current

// when menu is open and clicked inside menu, A is expected to be false
// when menu is open and clicked outside menu, A is expected to be true
console.log('A', el && !el.contains(event.target))

el && !el.contains(event.target) && onClickAway(event)
}

document.addEventListener('click', handler)

return () => {
document.removeEventListener('click', handler)
}
}, [onClickAway, ref])
}

export default function App() {
const [open, setOpen] = useState(false)

const menuRef = useRef(null)

const onClickAway = useCallback(() => {
console.log('click away, open', open)
if (open) {
setOpen(false)
}
}, [open])

useClickAway(menuRef, onClickAway)

return (
<div>
<div id="click-me" onClick={() => setOpen(true)}>
Open Menu
</div>
{open && (
<div ref={menuRef} id="the-menu">
<Link href="/">some link</Link>
</div>
)}
</div>
)
}
13 changes: 13 additions & 0 deletions test/integration/link-ref/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,15 @@ const didPrefetch = async (pathname) => {
await browser.close()
}

function runCommonTests() {
// See https://github.com/vercel/next.js/issues/18437
it('should not have a race condition with a click handler', async () => {
const browser = await webdriver(appPort, '/click-away-race-condition')
await browser.elementByCss('#click-me').click()
await browser.waitForElementByCss('#the-menu')
})
}

describe('Invalid hrefs', () => {
describe('dev mode', () => {
beforeAll(async () => {
Expand All @@ -57,6 +66,8 @@ describe('Invalid hrefs', () => {
})
afterAll(() => killApp(app))

runCommonTests()

it('should not show error for function component with forwardRef', async () => {
await noError('/function')
})
Expand All @@ -82,6 +93,8 @@ describe('Invalid hrefs', () => {
})
afterAll(() => killApp(app))

runCommonTests()

it('should preload with forwardRef', async () => {
await didPrefetch('/function')
})
Expand Down

0 comments on commit c0ae204

Please sign in to comment.