Skip to content

Commit

Permalink
Revert "[image, link] fix ref merging for callback refs that return a…
Browse files Browse the repository at this point in the history
… cleanup function" (#68176)

Seems this is causing us to hit the following error our vercel-site

```sh
 ReferenceError: navigator is not defined
vercel-site:build:     at s (/vercel/path0/apps/vercel-site/.next/server/chunks-EDOwHfQ0FCxDTUqmP-83_/65787.js:161:168406)
vercel-site:build:     at u (/vercel/path0/apps/vercel-site/.next/server/chunks-EDOwHfQ0FCxDTUqmP-83_/65787.js:50:154810)
```

Reverts #68123
  • Loading branch information
ijjk committed Jul 26, 2024
1 parent 7b14e20 commit 7f677d1
Show file tree
Hide file tree
Showing 7 changed files with 61 additions and 218 deletions.
103 changes: 53 additions & 50 deletions packages/next/src/client/image-component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import { RouterContext } from '../shared/lib/router-context.shared-runtime'

// @ts-ignore - This is replaced by webpack alias
import defaultLoader from 'next/dist/shared/lib/image-loader'
import { useMergedRef } from './use-merged-ref'

// This is replaced by webpack define plugin
const configEnv = process.env.__NEXT_IMAGE_OPTS as any as ImageConfigComplete
Expand Down Expand Up @@ -207,54 +206,6 @@ const ImageElement = forwardRef<HTMLImageElement | null, ImageElementProps>(
},
forwardedRef
) => {
const ownRef = useCallback(
(img: ImgElementWithDataProp | null) => {
if (!img) {
return
}
if (onError) {
// If the image has an error before react hydrates, then the error is lost.
// The workaround is to wait until the image is mounted which is after hydration,
// then we set the src again to trigger the error handler (if there was an error).
// eslint-disable-next-line no-self-assign
img.src = img.src
}
if (process.env.NODE_ENV !== 'production') {
if (!src) {
console.error(`Image is missing required "src" property:`, img)
}
if (img.getAttribute('alt') === null) {
console.error(
`Image is missing required "alt" property. Please add Alternative Text to describe the image for screen readers and search engines.`
)
}
}
if (img.complete) {
handleLoading(
img,
placeholder,
onLoadRef,
onLoadingCompleteRef,
setBlurComplete,
unoptimized,
sizesInput
)
}
},
[
src,
placeholder,
onLoadRef,
onLoadingCompleteRef,
setBlurComplete,
onError,
unoptimized,
sizesInput,
]
)

const ref = useMergedRef(forwardedRef, ownRef)

return (
<img
{...rest}
Expand All @@ -278,7 +229,59 @@ const ImageElement = forwardRef<HTMLImageElement | null, ImageElementProps>(
sizes={sizes}
srcSet={srcSet}
src={src}
ref={ref}
ref={useCallback(
(img: ImgElementWithDataProp | null) => {
if (forwardedRef) {
if (typeof forwardedRef === 'function') forwardedRef(img)
else if (typeof forwardedRef === 'object') {
// @ts-ignore - .current is read only it's usually assigned by react internally
forwardedRef.current = img
}
}
if (!img) {
return
}
if (onError) {
// If the image has an error before react hydrates, then the error is lost.
// The workaround is to wait until the image is mounted which is after hydration,
// then we set the src again to trigger the error handler (if there was an error).
// eslint-disable-next-line no-self-assign
img.src = img.src
}
if (process.env.NODE_ENV !== 'production') {
if (!src) {
console.error(`Image is missing required "src" property:`, img)
}
if (img.getAttribute('alt') === null) {
console.error(
`Image is missing required "alt" property. Please add Alternative Text to describe the image for screen readers and search engines.`
)
}
}
if (img.complete) {
handleLoading(
img,
placeholder,
onLoadRef,
onLoadingCompleteRef,
setBlurComplete,
unoptimized,
sizesInput
)
}
},
[
src,
placeholder,
onLoadRef,
onLoadingCompleteRef,
setBlurComplete,
onError,
unoptimized,
sizesInput,
forwardedRef,
]
)}
onLoad={(event) => {
const img = event.currentTarget as ImgElementWithDataProp
handleLoading(
Expand Down
13 changes: 8 additions & 5 deletions packages/next/src/client/link.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import { useIntersection } from './use-intersection'
import { getDomainLocale } from './get-domain-locale'
import { addBasePath } from './add-base-path'
import { PrefetchKind } from './components/router-reducer/router-reducer-types'
import { useMergedRef } from './use-merged-ref'

type Url = string | UrlObject
type RequiredKeys<T> = {
Expand Down Expand Up @@ -547,7 +546,7 @@ const Link = React.forwardRef<HTMLAnchorElement, LinkPropsReal>(
rootMargin: '200px',
})

const setIntersectionWithResetRef = React.useCallback(
const setRef = React.useCallback(
(el: Element) => {
// Before the link getting observed, check if visible state need to be reset
if (previousAs.current !== as || previousHref.current !== href) {
Expand All @@ -557,12 +556,16 @@ const Link = React.forwardRef<HTMLAnchorElement, LinkPropsReal>(
}

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

const setRef = useMergedRef(setIntersectionWithResetRef, childRef)

// Prefetch the URL if we haven't already and it's visible.
React.useEffect(() => {
// in dev, we only prefetch on hover to avoid wasting resources as the prefetch will trigger compiling the page.
Expand Down
46 changes: 0 additions & 46 deletions packages/next/src/client/use-merged-ref.ts

This file was deleted.

52 changes: 0 additions & 52 deletions test/integration/link-ref/pages/child-ref-func-cleanup.js

This file was deleted.

8 changes: 0 additions & 8 deletions test/integration/link-ref/test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,6 @@ describe('Invalid hrefs', () => {
it('should handle child ref that is a function', async () => {
await noError('/child-ref-func')
})

it('should handle child ref that is a function that returns a cleanup function', async () => {
await noError('/child-ref-func-cleanup')
})
}
)
;(process.env.TURBOPACK_DEV ? describe.skip : describe)(
Expand All @@ -113,10 +109,6 @@ describe('Invalid hrefs', () => {
it('should preload with child ref with function', async () => {
await didPrefetch('/child-ref-func')
})

it('should preload with child ref with function that returns a cleanup function', async () => {
await didPrefetch('/child-ref-func-cleanup')
})
}
)
})
43 changes: 0 additions & 43 deletions test/integration/next-image-new/app-dir/app/ref-cleanup/page.js

This file was deleted.

14 changes: 0 additions & 14 deletions test/integration/next-image-new/app-dir/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import {
nextBuild,
nextStart,
renderViaHTTP,
retry,
waitFor,
} from 'next-test-utils'
import webdriver from 'next-webdriver'
Expand Down Expand Up @@ -1575,19 +1574,6 @@ function runTests(mode) {
}
}
})

it('should call callback ref cleanups when unmounting', async () => {
const browser = await webdriver(appPort, '/ref-cleanup')
const getLogs = async () => (await browser.log()).map((log) => log.message)

await retry(async () => {
expect(await getLogs()).toContain('callback ref was cleaned up')
})

expect(await getLogs()).not.toContain(
'callback refs that returned a cleanup should never be called with null'
)
})
}

describe('Image Component Default Tests', () => {
Expand Down

0 comments on commit 7f677d1

Please sign in to comment.