Fix: Added the content-disposition header #27521

LetItRock · 2021-07-26T22:55:53Z

In this PR I've added the Content-Disposition header to the response of the image /_next/image route. That header is used by the browser when showing the dialog for the Save image as... action.

There are some differences between the browsers, ex:
When requesting the image test.jpg, the response header Content-Type: image/webp - in FF the filename from the Save image as... dialog would be test.webp but in Chrome test.jpg even if the Content-Disposition: inline; filename="test.webp" is present in the headers. The same about png images, the rest types are fine. It looks like FF is checking the Content-Type for the extension but the Chrome does not and is doing another type of check.

Fixes #26737

Bug

Related issues linked using fixes #number
Integration tests added
Errors have helpful link attached, see contributing.md

Documentation / Examples

Make sure the linting passes

…t/image route;

styfle · 2021-07-26T23:00:53Z

test/integration/image-optimizer/test/index.test.js

@@ -1108,4 +1108,46 @@ describe('Image Optimizer', () => {

    setupTests(true)
  })
+
+  describe('content disposition header', () => {


I think we can utilize the existing tests and add a check for content-disposition just above content-type since nearly every test is checking headers

next.js/test/integration/image-optimizer/test/index.test.js

Line 62 in 2a5dd0b

expect(res.headers.get('content-type')).toContain('image/gif')

yeah, I thought about it at the beginning but noticed that there are no cases for external URLs like in lines 1135-1137

There is an external URL test here

next.js/test/integration/image-optimizer/test/index.test.js

Lines 355 to 356 in 2a5dd0b

const url =

'https://image-optimization-test.vercel.app/png-as-octet-stream'

packages/next/server/image-optimizer.ts

styfle · 2021-07-26T23:07:14Z

test/integration/image-optimizer/test/index.test.js

+      ${'https://image-optimization-test.vercel.app/api/frog.jpg'}        | ${`inline; filename="frog.webp"`}
+      ${'https://image-optimization-test.vercel.app/api/frog.png'}        | ${`inline; filename="frog.webp"`}
+      ${'https://image-optimization-test.vercel.app/api/vercel.svg'}      | ${`inline; filename="vercel.svg"`}
+      ${'https://image-optimization-test.vercel.app/png-as-octet-stream'} | ${`inline; filename="png-as-octet-stream.webp"`}


I'm not so sure about changing the octet stream to webp 🤔

the content-type that is returned in the response is image/webp

Oh yes, that was added in #26705

I think keeping .webp extension is fine here. Lets add it to the existing test here:

next.js/test/integration/image-optimizer/test/index.test.js

Lines 355 to 356 in 2a5dd0b

const url =

'https://image-optimization-test.vercel.app/png-as-octet-stream'

packages/next/server/image-optimizer.ts

ijjk · 2021-07-26T23:13:44Z

Failing test suites

Commit: 2a5dd0b

test/integration/catches-missing-getStaticProps/test/index.test.js

Catches Missing getStaticProps > should catch it in server build mode

Expand output

● Catches Missing getStaticProps › should catch it in server build mode

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

  29 |   })
  30 |
> 31 |   it('should catch it in server build mode', async () => {
     |   ^
  32 |     await fs.remove(nextConfig)
  33 |     const { stderr } = await nextBuild(appDir, [], {
  34 |       stderr: true,

  at integration/catches-missing-getStaticProps/test/index.test.js:31:3
  at Object.<anonymous> (integration/catches-missing-getStaticProps/test/index.test.js:19:1)

…27521;

LetItRock · 2021-07-27T14:29:27Z

hi @styfle! I've updated the tests ;)

…mage_optimizer_content_disposition_header

test/integration/image-optimizer/test/index.test.js

packages/next/server/image-optimizer.ts

test/integration/image-optimizer/test/index.test.js

…ated the tests accordingly;

LetItRock · 2021-07-27T21:32:27Z

@styfle updated PR with your suggestions, thanks!

…mage_optimizer_content_disposition_header

styfle

Thanks!

…ader

ijjk · 2021-07-27T22:05:05Z

Stats from current PR

Default Build (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	LetItRock/next.js fix/image_optimizer_content_disposition_header	Change
buildDuration	15.6s	15.4s	-154ms
buildDurationCached	3.6s	3.5s	-56ms
nodeModulesSize	50.4 MB	50.4 MB	⚠️ +1.92 kB

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	LetItRock/next.js fix/image_optimizer_content_disposition_header	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.764	2.785	⚠️ +0.02
/ avg req/sec	904.33	897.58	⚠️ -6.75
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.69	1.695	⚠️ +0.01
/error-in-render avg req/sec	1479.33	1474.64	⚠️ -4.69

Client Bundles (main, webpack, commons)

	vercel/next.js canary	LetItRock/next.js fix/image_optimizer_content_disposition_header	Change
359.HASH.js gzip	2.96 kB	2.96 kB	✓
745.HASH.js gzip	180 B	180 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	21 kB	21 kB	✓
webpack-HASH.js gzip	1.53 kB	1.53 kB	✓
Overall change	67.9 kB	67.9 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	LetItRock/next.js fix/image_optimizer_content_disposition_header	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	LetItRock/next.js fix/image_optimizer_content_disposition_header	Change
_app-HASH.js gzip	803 B	803 B	✓
_error-HASH.js gzip	3.06 kB	3.06 kB	✓
amp-HASH.js gzip	554 B	554 B	✓
css-HASH.js gzip	329 B	329 B	✓
dynamic-HASH.js gzip	2.52 kB	2.52 kB	✓
head-HASH.js gzip	2.28 kB	2.28 kB	✓
hooks-HASH.js gzip	903 B	903 B	✓
image-HASH.js gzip	5.63 kB	5.63 kB	✓
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	1.66 kB	1.66 kB	✓
routerDirect..HASH.js gzip	319 B	319 B	✓
script-HASH.js gzip	387 B	387 B	✓
withRouter-HASH.js gzip	320 B	320 B	✓
bb14e60e810b..30f.css gzip	125 B	125 B	✓
Overall change	19.1 kB	19.1 kB	✓

Client Build Manifests

	vercel/next.js canary	LetItRock/next.js fix/image_optimizer_content_disposition_header	Change
_buildManifest.js gzip	489 B	489 B	✓
Overall change	489 B	489 B	✓

Rendered Page Sizes

	vercel/next.js canary	LetItRock/next.js fix/image_optimizer_content_disposition_header	Change
index.html gzip	531 B	531 B	✓
link.html gzip	544 B	544 B	✓
withRouter.html gzip	525 B	525 B	✓
Overall change	1.6 kB	1.6 kB	✓

Webpack 4 Mode

General Overall increase ⚠️

	vercel/next.js canary	LetItRock/next.js fix/image_optimizer_content_disposition_header	Change
buildDuration	12.1s	12.1s	-36ms
buildDurationCached	4.9s	4.9s	⚠️ +24ms
nodeModulesSize	50.4 MB	50.4 MB	⚠️ +2.01 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	LetItRock/next.js fix/image_optimizer_content_disposition_header	Change
/ failed reqs	0	0	✓
/ total time (seconds)	2.763	2.79	⚠️ +0.03
/ avg req/sec	904.81	896.15	⚠️ -8.66
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.735	1.72	-0.02
/error-in-render avg req/sec	1440.71	1453.34	+12.63

Client Bundles (main, webpack, commons)

	vercel/next.js canary	LetItRock/next.js fix/image_optimizer_content_disposition_header	Change
17.HASH.js gzip	2.98 kB	2.98 kB	✓
18.HASH.js gzip	185 B	185 B	✓
677f882d2ed8..HASH.js gzip	13.8 kB	13.8 kB	✓
framework.HASH.js gzip	41.9 kB	41.9 kB	✓
main-HASH.js gzip	8.4 kB	8.4 kB	✓
webpack-HASH.js gzip	1.22 kB	1.22 kB	✓
Overall change	68.5 kB	68.5 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	LetItRock/next.js fix/image_optimizer_content_disposition_header	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	LetItRock/next.js fix/image_optimizer_content_disposition_header	Change
_app-HASH.js gzip	791 B	791 B	✓
_error-HASH.js gzip	3.76 kB	3.76 kB	✓
amp-HASH.js gzip	552 B	552 B	✓
css-HASH.js gzip	333 B	333 B	✓
dynamic-HASH.js gzip	2.71 kB	2.71 kB	✓
head-HASH.js gzip	2.97 kB	2.97 kB	✓
hooks-HASH.js gzip	911 B	911 B	✓
index-HASH.js gzip	231 B	231 B	✓
link-HASH.js gzip	1.64 kB	1.64 kB	✓
routerDirect..HASH.js gzip	298 B	298 B	✓
script-HASH.js gzip	3.02 kB	3.02 kB	✓
withRouter-HASH.js gzip	294 B	294 B	✓
e025d2764813..52f.css gzip	125 B	125 B	✓
Overall change	17.6 kB	17.6 kB	✓

Client Build Manifests

	vercel/next.js canary	LetItRock/next.js fix/image_optimizer_content_disposition_header	Change
_buildManifest.js gzip	500 B	500 B	✓
Overall change	500 B	500 B	✓

Rendered Page Sizes

	vercel/next.js canary	LetItRock/next.js fix/image_optimizer_content_disposition_header	Change
index.html gzip	577 B	577 B	✓
link.html gzip	588 B	588 B	✓
withRouter.html gzip	569 B	569 B	✓
Overall change	1.73 kB	1.73 kB	✓

Commit: 596b7d2

In this PR I've added the `Content-Disposition` header to the response of the image `/_next/image` route. That header is used by the browser when showing the dialog for the `Save image as...` action. There are some differences between the browsers, ex: When requesting the image `test.jpg`, the response header `Content-Type: image/webp` - in FF the filename from the `Save image as...` dialog would be `test.webp` but in Chrome `test.jpg` even if the `Content-Disposition: inline; filename="test.webp"` is present in the headers. The same about png images, the rest types are fine. It looks like FF is checking the `Content-Type` for the extension but the Chrome does not and is doing another type of check. Fixes vercel#26737 ## Bug - [x] Related issues linked using `fixes #number` - [x] Integration tests added - [ ] Errors have helpful link attached, see `contributing.md` ## Documentation / Examples - [x] Make sure the linting passes

- added the content-disposition header to the image response of /_nex…

2a5dd0b

…t/image route;

LetItRock requested review from huozhi, ijjk, padmaia, shuding, styfle and timneutkens as code owners July 26, 2021 22:55

ijjk added the type: next label Jul 26, 2021

styfle reviewed Jul 26, 2021

View reviewed changes

LetItRock commented Jul 26, 2021

View reviewed changes

packages/next/server/image-optimizer.ts Show resolved Hide resolved

This comment has been minimized.

Sign in to view

styfle reviewed Jul 26, 2021

View reviewed changes

packages/next/server/image-optimizer.ts Show resolved Hide resolved

- changed the tests according to the suggestions from the PR: vercel#…

6cf4553

…27521;

Merge branch 'canary' of https://github.com/vercel/next.js into fix/i…

fe29011

…mage_optimizer_content_disposition_header

This comment has been minimized.

Sign in to view

styfle reviewed Jul 27, 2021

View reviewed changes

test/integration/image-optimizer/test/index.test.js Outdated Show resolved Hide resolved

styfle reviewed Jul 27, 2021

View reviewed changes

packages/next/server/image-optimizer.ts Outdated Show resolved Hide resolved

styfle reviewed Jul 27, 2021

View reviewed changes

test/integration/image-optimizer/test/index.test.js Outdated Show resolved Hide resolved

styfle reviewed Jul 27, 2021

View reviewed changes

test/integration/image-optimizer/test/index.test.js Outdated Show resolved Hide resolved

- for the 304 we should not set the content-disposition header; - upd…

7b65457

…ated the tests accordingly;

Merge branch 'canary' of https://github.com/vercel/next.js into fix/i…

6dea0a4

…mage_optimizer_content_disposition_header

styfle previously approved these changes Jul 27, 2021

View reviewed changes

Merge branch 'canary' into fix/image_optimizer_content_disposition_he…

307c897

…ader

This comment has been minimized.

Sign in to view

Update tests

596b7d2

styfle dismissed their stale review via 596b7d2 July 27, 2021 21:53

styfle approved these changes Jul 27, 2021

View reviewed changes

kodiakhq bot merged commit 36b81f9 into vercel:canary Jul 27, 2021

LetItRock deleted the fix/image_optimizer_content_disposition_header branch July 28, 2021 21:58

vercel locked as resolved and limited conversation to collaborators Jan 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Added the content-disposition header #27521

Fix: Added the content-disposition header #27521

LetItRock commented Jul 26, 2021

styfle Jul 26, 2021

LetItRock Jul 26, 2021 •

edited

Loading

styfle Jul 26, 2021

This comment has been minimized.

styfle Jul 26, 2021

LetItRock Jul 26, 2021

styfle Jul 26, 2021

ijjk commented Jul 26, 2021

LetItRock commented Jul 27, 2021

This comment has been minimized.

LetItRock commented Jul 27, 2021

styfle left a comment

This comment has been minimized.

ijjk commented Jul 27, 2021

	const url =
	'https://image-optimization-test.vercel.app/png-as-octet-stream'

Fix: Added the content-disposition header #27521

Fix: Added the content-disposition header #27521

Conversation

LetItRock commented Jul 26, 2021

Bug

Documentation / Examples

styfle Jul 26, 2021

Choose a reason for hiding this comment

LetItRock Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

styfle Jul 26, 2021

Choose a reason for hiding this comment

This comment has been minimized.

styfle Jul 26, 2021

Choose a reason for hiding this comment

LetItRock Jul 26, 2021

Choose a reason for hiding this comment

styfle Jul 26, 2021

Choose a reason for hiding this comment

ijjk commented Jul 26, 2021

Failing test suites

LetItRock commented Jul 27, 2021

This comment has been minimized.

LetItRock commented Jul 27, 2021

styfle left a comment

Choose a reason for hiding this comment

This comment has been minimized.

ijjk commented Jul 27, 2021

Stats from current PR

LetItRock Jul 26, 2021 •

edited

Loading