chore: replace next/legacy/image with next/image #1722

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

Jump to bottom

Merged

kodiakhq merged 10 commits into main from next-js-images

Oct 28, 2022

nickytonline commented Oct 26, 2022 •

edited

Loading

Summary

Swaps next/legacy/image imports with next/image imports. I've also added a test to verify that a broken image due to image configuration in Next.js should error out if the host is not allowed.

Test plan

All the demo sites should build fine and all the tests should pass.

Relevant links (GitHub issues, Notion docs, etc.) or a picture of cute animal

Closes #1721

Standard checks:

Check the Deploy Preview's Demo site for your PR's functionality
Add docs when necessary

🧪 Once merged, make sure to update the version if needed and that it was published correctly.

netlify bot commented Oct 26, 2022 •

edited

Loading

✅ Deploy Preview for netlify-plugin-nextjs-nx-monorepo-demo ready!

Name	Link
🔨 Latest commit	`9c2689c`
🔍 Latest deploy log	https://app.netlify.com/sites/netlify-plugin-nextjs-nx-monorepo-demo/deploys/635c22ef84e710000832ab14
😎 Deploy Preview	https://deploy-preview-1722--netlify-plugin-nextjs-nx-monorepo-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify bot commented Oct 26, 2022 •

edited

Loading

✅ Deploy Preview for netlify-plugin-nextjs-export-demo ready!

Name	Link
🔨 Latest commit	`9c2689c`
🔍 Latest deploy log	https://app.netlify.com/sites/netlify-plugin-nextjs-export-demo/deploys/635c22efc32b0f000877b658
😎 Deploy Preview	https://deploy-preview-1722--netlify-plugin-nextjs-export-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify bot commented Oct 26, 2022 •

edited

Loading

✅ Deploy Preview for netlify-plugin-nextjs-static-root-demo ready!

Name	Link
🔨 Latest commit	`9c2689c`
🔍 Latest deploy log	https://app.netlify.com/sites/netlify-plugin-nextjs-static-root-demo/deploys/635c22eff55cae00085bc494
😎 Deploy Preview	https://deploy-preview-1722--netlify-plugin-nextjs-static-root-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify bot commented Oct 26, 2022 •

edited

Loading

✅ Deploy Preview for next-plugin-edge-middleware ready!

Name	Link
🔨 Latest commit	`9c2689c`
🔍 Latest deploy log	https://app.netlify.com/sites/next-plugin-edge-middleware/deploys/635c22ef83f4200008993908
😎 Deploy Preview	https://deploy-preview-1722--next-plugin-edge-middleware.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

github-actions bot added the type: chore label

netlify bot commented Oct 26, 2022 •

edited

Loading

✅ Deploy Preview for netlify-plugin-nextjs-next-auth-demo ready!

Name	Link
🔨 Latest commit	`9c2689c`
🔍 Latest deploy log	https://app.netlify.com/sites/netlify-plugin-nextjs-next-auth-demo/deploys/635c22efc6311b0009fc5524
😎 Deploy Preview	https://deploy-preview-1722--netlify-plugin-nextjs-next-auth-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify bot commented Oct 26, 2022 •

edited

Loading

✅ Deploy Preview for next-plugin-canary ready!

Name	Link
🔨 Latest commit	`9c2689c`
🔍 Latest deploy log	https://app.netlify.com/sites/next-plugin-canary/deploys/635c22efc18712000886413c
😎 Deploy Preview	https://deploy-preview-1722--next-plugin-canary.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify bot commented Oct 26, 2022 •

edited

Loading

✅ Deploy Preview for next-i18next-demo ready!

Name	Link
🔨 Latest commit	`9c2689c`
🔍 Latest deploy log	https://app.netlify.com/sites/next-i18next-demo/deploys/635c22ef2dd4930008b75b30
😎 Deploy Preview	https://deploy-preview-1722--next-i18next-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify bot commented Oct 26, 2022 •

edited

Loading

✅ Deploy Preview for nextjs-plugin-custom-routes-demo ready!

Name	Link
🔨 Latest commit	`9c2689c`
🔍 Latest deploy log	https://app.netlify.com/sites/nextjs-plugin-custom-routes-demo/deploys/635c22ef9d8df500080ef430
😎 Deploy Preview	https://deploy-preview-1722--nextjs-plugin-custom-routes-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify bot commented Oct 26, 2022 •

edited

Loading

✅ Deploy Preview for netlify-plugin-nextjs-demo ready!

Name	Link
🔨 Latest commit	`9c2689c`
🔍 Latest deploy log	https://app.netlify.com/sites/netlify-plugin-nextjs-demo/deploys/635c22ef9974d5000806c11a
😎 Deploy Preview	https://deploy-preview-1722--netlify-plugin-nextjs-demo.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

nickytonline closed this

nickytonline force-pushed the next-js-images branch from 4067b67 to 5e906fa Compare

October 26, 2022 20:07

nickytonline changed the title ~~chore: removed unused import~~ wip

nickytonline reopened this

nickytonline changed the base branch from main to mk/canary-demo

October 27, 2022 18:08

nickytonline changed the title ~~wip~~ Replace next/legacy/image with next/image

nickytonline force-pushed the next-js-images branch from 2b583e5 to faeabe8 Compare

October 27, 2022 18:19

nickytonline marked this pull request as ready for review

October 27, 2022 18:21

nickytonline requested a review from a team

October 27, 2022 18:21

nickytonline marked this pull request as draft

October 27, 2022 18:22

nickytonline marked this pull request as ready for review

October 27, 2022 19:03

nickytonline commented

View reviewed changes

cypress/integration/default/images.spec.ts Show resolved Hide resolved

cypress/integration/default/images.spec.ts Outdated

-                      'image has natural width'
-                    ).to.be.greaterThan(0)
-                  })
+                  cy.findByRole('img', { name: /tawny frogmouth/i })

Author

nickytonline Oct 27, 2022

prettier in action

demos/default/next.config.js

@@ @@ -71,7 +71,7 @@ module.exports = { @@
                 },
                 // https://nextjs.org/docs/basic-features/image-optimization#domains
                 images: {
-                  domains: ['raw.githubusercontent.com', 'upload.wikimedia.org'],

Author

nickytonline Oct 27, 2022

Removed 'upload.wikimedia.org', as it was added a few weeks ago to prevent the /images page in the default demo from breaking due to Next.js throwing when the host is not supported for an image.

demos/default/pages/image.js Show resolved Hide resolved

Contributor

ascorbic commented Oct 27, 2022

Why is this based off the app dir branch?

ericapisani suggested changes

View reviewed changes

demos/default/pages/image.js Show resolved Hide resolved

Author

nickytonline commented Oct 27, 2022

Why is this based off the app dir branch?

@ascorbic, I could've swarn these changes were related to the work in your branch, but pulling latest from main shows that is clearly not the case. Thursdays.

nickytonline changed the base branch from mk/canary-demo to main

October 27, 2022 19:26

nickytonline added 3 commits

October 27, 2022 15:28


          chore: removed unused import

f5e94a5


          chore: removed broken image that was no longer broken from main image…

1de22f8

…s demo page


          test: add e2e test to test broken image error

ab989a5

nickytonline force-pushed the next-js-images branch from beadf65 to ab989a5 Compare

October 27, 2022 19:28


          chore: fixed image import

262f0c6

nickytonline changed the title ~~Replace next/legacy/image with next/image~~ fix : replace next/legacy/image with next/image

github-actions bot added the type: bug label

Contributor

ascorbic commented Oct 27, 2022

I think the confusion was because it's Next 13 stuff - but it was the other PR that we already merged.

nickytonline changed the title ~~fix : replace next/legacy/image with next/image~~ chore: replace next/legacy/image with next/image

nickytonline self-assigned this


          test: updated test validating image config

6da578a

nickytonline commented

View reviewed changes

test/index.js

@@ @@ -615,7 +614,7 @@ describe('onBuild()', () => { @@
                   const imageConfigPath = path.join(constants.INTERNAL_FUNCTIONS_SRC, IMAGE_FUNCTION_NAME, 'imageconfig.json')
                   const imageConfigJson = await readJson(imageConfigPath)
-                  expect(imageConfigJson.domains.length).toBe(2)
+                  expect(imageConfigJson.domains.length).toBe(1)

Author

nickytonline Oct 27, 2022

The wikipedia domain was removed, so now there is only one domain. See https://github.com/netlify/next-runtime/pull/1722/files#diff-f3bc73930689da34c69d4dd219fb06b3965a303551a6b4fe527b2492534511e6L74

test/index.js Show resolved Hide resolved

test/index.js Show resolved Hide resolved

nickytonline requested a review from ericapisani

October 27, 2022 19:41

nickytonline commented

View reviewed changes

cypress/integration/default/images.spec.ts Outdated

-                    ).to.equal('0px')
+                it('should show throw if an image is not on the domains or remotePatterns allowlist', () => {
+                  cy.request('/broken-image').then((response) => {
+                    expect(response.status).to.eq(200)

Author

nickytonline Oct 27, 2022

The image is broken, but the page still loads with the error message which is why this is an HTTTP 200

nickytonline requested a review from a team

October 27, 2022 19:56

nickytonline force-pushed the next-js-images branch from 3289bb1 to 6da578a Compare

October 27, 2022 21:01

nickytonline added 2 commits

October 27, 2022 17:17


          test: fixed broken image test

3d516e4


          test: fixed broken image test as it's different when deployed

dbd8558

nickytonline force-pushed the next-js-images branch 3 times, most recently from 0e2cb05 to dbd8558 Compare

October 28, 2022 02:20

ascorbic reviewed

View reviewed changes

demos/default/pages/broken-image.tsx Show resolved Hide resolved


          Merge branch 'main' into next-js-images

c865a9f

nickytonline added the automerge label

nickytonline force-pushed the next-js-images branch 2 times, most recently from 19be202 to b16d251 Compare

October 28, 2022 14:26


          test: used a real image to test an image not on the Next.js allow list

5cf563d

nickytonline force-pushed the next-js-images branch from b16d251 to 5cf563d Compare

October 28, 2022 14:42

ascorbic approved these changes

View reviewed changes

Author

nickytonline commented Oct 28, 2022

This was passing minus the preview spec flakiness, but I've rerun things and atm it's failing because things are timing out on waiting for sites to build. I'll let it rest for a bit and rerun later this afternoon.


          Merge branch 'main' into next-js-images

9c2689c

ericapisani approved these changes

View reviewed changes

kodiakhq bot merged commit bcd7c11 into main

kodiakhq bot deleted the next-js-images branch

October 28, 2022 18:58

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge type: bug type: chore