Fix next/image noscript src path with loaders #24011

valse · 2021-04-13T09:13:52Z

In the noscript img version the correct src and sizes attributes are overwritten by not necessary inline declaration; in particular using the loaders the src attribute not take the right absolute path. I found this issue using a custom loader and because my site didn't indexing any images on the Google image search.

Fixes #24277

In the noscript img version the correct `src` and `sizes` attributes are overwritten by not necessary inline declaration; in particular using the loaders the `src` attribute not take the right absolute path. I found this issue using a custom loader and because my site didn't indexing any images on the Google image search.

timneutkens

This is missing an integration test and a fixes marker, there's a related issue to this change right?

valse · 2021-04-18T18:42:29Z

This is missing an integration test and a fixes marker, there's a related issue to this change right?

I fixed the formatting issue, thanks.

I think that this is one of the next priority issue because the final rendered source code it's wrong and it could be why the images are not correctly crawled.

valse · 2021-04-20T16:57:15Z

This is linked to this issue #24277

timneutkens · 2021-04-21T10:23:16Z

I think that this is one of the next priority issue because the final rendered source code it's wrong and it could be why the images are not correctly crawled.

We can't merge PRs (especially bugfixes) without tests. As said integration tests should be added for this change to prevent regressions in the future 👍

timneutkens · 2021-06-08T07:26:16Z

👋 reminder that this can't be merged without tests, can you please add them 🙏

valse · 2021-06-08T07:36:45Z

👋 reminder that this can't be merged without tests, can you please add them 🙏

Hi @timneutkens ... could you please give me some suggestion to how add them: I never added tests before and I don't know if I must create a new test file or improve an existing one, thanks!

timneutkens · 2021-06-08T07:51:11Z

You can check test/integration/image-component for a reference point. For example adding a new directory based on the basic directory.

valse · 2021-06-08T08:44:14Z

You can check test/integration/image-component for a reference point. For example adding a new directory based on the basic directory.

Ok thanks... I'm new in the testing world and I can't understand how to run a single test... for testing purpose :P

timneutkens · 2021-06-08T08:54:52Z

No worries! It's explained in the contributing doc: https://github.com/vercel/next.js/blob/canary/contributing.md#to-run-tests

valse · 2021-06-16T18:25:53Z

No worries! It's explained in the contributing doc: https://github.com/vercel/next.js/blob/canary/contributing.md#to-run-tests

@timneutkens I added some tests few days ago... Are they ok? Thanks!

timneutkens

Looks good now! Thanks!

ijjk · 2021-06-16T19:58:48Z

Failing test suites

Commit: 6ec8796

test/integration/basic/test/index.test.js

Basic Features > should polyfill Node.js modules

Expand output

● Basic Features › should polyfill Node.js modules

thrown: "Exceeded timeout of 300000 ms for a test.
Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

  35 |   afterAll(() => killApp(context.server))
  36 |
> 37 |   it('should polyfill Node.js modules', async () => {
     |   ^
  38 |     const browser = await webdriver(context.appPort, '/node-browser-polyfills')
  39 |
  40 |     console.error({

  at integration/basic/test/index.test.js:37:3
  at Object.<anonymous> (integration/basic/test/index.test.js:18:1)

ijjk · 2021-06-16T20:35:53Z

Stats from current PR

Default Server Mode (Decrease detected ✓)

General Overall decrease ✓

	vercel/next.js canary	valse/next.js patch-2	Change
buildDuration	10.6s	10.3s	-235ms
buildDurationCached	2.6s	2.7s	⚠️ +96ms
nodeModulesSize	46.4 MB	46.4 MB	-113 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	valse/next.js patch-2	Change
/ failed reqs	0	0	✓
/ total time (seconds)	1.902	2.057	⚠️ +0.16
/ avg req/sec	1314.31	1215.58	⚠️ -98.73
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.094	1.132	⚠️ +0.04
/error-in-render avg req/sec	2285.66	2207.98	⚠️ -77.68

Client Bundles (main, webpack, commons)

	vercel/next.js canary	valse/next.js patch-2	Change
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	20.2 kB	20.2 kB	✓
webpack-HASH.js gzip	804 B	804 B	✓
Overall change	63 kB	63 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	valse/next.js patch-2	Change
polyfills-HASH.js gzip	31.1 kB	31.1 kB	✓
Overall change	31.1 kB	31.1 kB	✓

Client Pages

	vercel/next.js canary	valse/next.js patch-2	Change
_app-HASH.js gzip	801 B	801 B	✓
_error-HASH.js gzip	3.17 kB	3.17 kB	✓
amp-HASH.js gzip	527 B	527 B	✓
css-HASH.js gzip	329 B	329 B	✓
hooks-HASH.js gzip	903 B	903 B	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	1.65 kB	1.65 kB	✓
routerDirect..HASH.js gzip	322 B	322 B	✓
withRouter-HASH.js gzip	320 B	320 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	8.41 kB	8.41 kB	✓

Client Build Manifests

	vercel/next.js canary	valse/next.js patch-2	Change
_buildManifest.js gzip	391 B	391 B	✓
Overall change	391 B	391 B	✓

Rendered Page Sizes

	vercel/next.js canary	valse/next.js patch-2	Change
index.html gzip	523 B	523 B	✓
link.html gzip	536 B	536 B	✓
withRouter.html gzip	516 B	516 B	✓
Overall change	1.57 kB	1.57 kB	✓

Serverless Mode (Decrease detected ✓)

General Overall decrease ✓

	vercel/next.js canary	valse/next.js patch-2	Change
buildDuration	12.4s	11.9s	-424ms
buildDurationCached	3.4s	3.6s	⚠️ +161ms
nodeModulesSize	46.4 MB	46.4 MB	-113 B

Client Bundles (main, webpack, commons)

	vercel/next.js canary	valse/next.js patch-2	Change
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	20.2 kB	20.2 kB	✓
webpack-HASH.js gzip	804 B	804 B	✓
Overall change	63 kB	63 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	valse/next.js patch-2	Change
polyfills-HASH.js gzip	31.1 kB	31.1 kB	✓
Overall change	31.1 kB	31.1 kB	✓

Client Pages

	vercel/next.js canary	valse/next.js patch-2	Change
_app-HASH.js gzip	801 B	801 B	✓
_error-HASH.js gzip	3.17 kB	3.17 kB	✓
amp-HASH.js gzip	527 B	527 B	✓
css-HASH.js gzip	329 B	329 B	✓
hooks-HASH.js gzip	903 B	903 B	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	1.65 kB	1.65 kB	✓
routerDirect..HASH.js gzip	322 B	322 B	✓
withRouter-HASH.js gzip	320 B	320 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	8.41 kB	8.41 kB	✓

Client Build Manifests

	vercel/next.js canary	valse/next.js patch-2	Change
_buildManifest.js gzip	391 B	391 B	✓
Overall change	391 B	391 B	✓

Serverless bundles

	vercel/next.js canary	valse/next.js patch-2	Change
_error.js	16.9 kB	16.9 kB	✓
404.html	1.98 kB	1.98 kB	✓
500.html	1.96 kB	1.96 kB	✓
amp.amp.html	10.8 kB	10.8 kB	✓
amp.html	1.17 kB	1.17 kB	✓
css.html	1.35 kB	1.35 kB	✓
hooks.html	1.23 kB	1.23 kB	✓
index.js	17.2 kB	17.2 kB	✓
link.js	17.5 kB	17.5 kB	✓
routerDirect.js	17.3 kB	17.3 kB	✓
withRouter.js	17.3 kB	17.3 kB	✓
Overall change	105 kB	105 kB	✓

Webpack 4 Mode (Decrease detected ✓)

General Overall decrease ✓

	vercel/next.js canary	valse/next.js patch-2	Change
buildDuration	9.5s	9.5s	⚠️ +6ms
buildDurationCached	3.8s	3.9s	⚠️ +124ms
nodeModulesSize	46.4 MB	46.4 MB	-113 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	valse/next.js patch-2	Change
/ failed reqs	0	0	✓
/ total time (seconds)	1.902	1.943	⚠️ +0.04
/ avg req/sec	1314.28	1286.63	⚠️ -27.65
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.075	1.172	⚠️ +0.1
/error-in-render avg req/sec	2325.03	2133.78	⚠️ -191.25

Client Bundles (main, webpack, commons)

	vercel/next.js canary	valse/next.js patch-2	Change
677f882d2ed8..HASH.js gzip	13.3 kB	13.3 kB	✓
framework.HASH.js gzip	41.8 kB	41.8 kB	✓
main-HASH.js gzip	7.99 kB	7.99 kB	✓
webpack-HASH.js gzip	751 B	751 B	✓
Overall change	63.8 kB	63.8 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	valse/next.js patch-2	Change
polyfills-HASH.js gzip	31.3 kB	31.3 kB	✓
Overall change	31.3 kB	31.3 kB	✓

Client Pages

	vercel/next.js canary	valse/next.js patch-2	Change
_app-HASH.js gzip	1.07 kB	1.07 kB	✓
_error-HASH.js gzip	3.84 kB	3.84 kB	✓
amp-HASH.js gzip	536 B	536 B	✓
css-HASH.js gzip	333 B	333 B	✓
hooks-HASH.js gzip	910 B	910 B	✓
index-HASH.js gzip	227 B	227 B	✓
link-HASH.js gzip	1.64 kB	1.64 kB	✓
routerDirect..HASH.js gzip	295 B	295 B	✓
withRouter-HASH.js gzip	292 B	292 B	✓
e025d2764813..52f.css gzip	125 B	125 B	✓
Overall change	9.28 kB	9.28 kB	✓

Client Build Manifests

	vercel/next.js canary	valse/next.js patch-2	Change
_buildManifest.js gzip	420 B	420 B	✓
Overall change	420 B	420 B	✓

Rendered Page Sizes

	vercel/next.js canary	valse/next.js patch-2	Change
index.html gzip	568 B	568 B	✓
link.html gzip	579 B	579 B	✓
withRouter.html gzip	561 B	561 B	✓
Overall change	1.71 kB	1.71 kB	✓

Commit: ab82c7a

In the `noscript` img version the correct `src` and `sizes` attributes are overwritten by not necessary inline declaration; in particular using the loaders the `src` attribute not take the right absolute path. I found this issue using a custom loader and because my site didn't indexing any images on the Google image search. Fixes vercel#24277

valse requested review from divmain, ijjk, lfades, shuding and timneutkens as code owners April 13, 2021 09:13

ijjk added the type: next label Apr 13, 2021