Don't import internally from types in `next-env.d.ts` by lfades · Pull Request #34394 · vercel/next.js

lfades · 2022-02-15T23:59:18Z

This PR fixes an issue where StaticImageData is declared inside a global declaration file (image-types/global.d.ts) but it's not available in the published package.

Having StaticImageData defined globally works when working inside the Next.js repo because TypeScript takes care of adding global declarations to the scope, however the same isn't true when the package is published, the declaration file doesn't get included by default and therefore using the StaticImageData fails inside the image component types.

With this change the type is now defined in the Image component and the global declaration file is importing it from there instead, this file is also excluded in tsconfig to make sure it doesn't break when developing inside the Next.js repo, because the import goes to /dist

Bug

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

styfle

Nice catch, thanks!

ijjk

Are we able to add a test case to prevent regressing on this? If the test needs to be run outside the Next.js repo we can use a production test which is isolated

lfades · 2022-02-16T01:33:13Z

@ijjk If the change is added in the future again it will be caught by our type checking before publishing next, because it's now excluded in tsconfig and so trying to import from there won't work, this is the same behavior as the *.d.ts files we have in the root.

I tried doing the same for types/global.d.ts but we're internally relying in the namespace changes we do to NodeJS, however there is no issue here because even though we rely on it, we don't rely on exported interfaces/types from it, so the generated declaration files don't care about it, unless of course an interface/type is added there and imported somewhere within the source, that's unlikely, however, we can add a note, wdyt?

lfades · 2022-02-16T16:28:46Z

-
 declare module '*.png' {
-  const content: StaticImageData
+  const content: import('../dist/client/image').StaticImageData


TIL if I add a root import in this file TS will no longer treat it as a global declaration file, but using import within the ambient declaration works

ijjk · 2022-02-16T16:39:38Z

Failing test suites

Commit: 3f68785

yarn testheadless test/production/next/jest/index.test.ts

next/jest > should work

Expand output

● next/jest › should work

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

   6 |   let next: NextInstance
   7 |
>  8 |   beforeAll(async () => {
     |   ^
   9 |     next = await createNext({
  10 |       files: {
  11 |         'components/comp.js': `

  at production/next/jest/index.test.ts:8:3
  at Object.<anonymous> (production/next/jest/index.test.ts:5:1)

● Test suite failed to run

TypeError: Cannot read property 'destroy' of undefined

  113 |     })
  114 |   })
> 115 |   afterAll(() => next.destroy())
      |                       ^
  116 |
  117 |   it('should work', async () => {
  118 |     const html = await renderViaHTTP(next.url, '/')

  at production/next/jest/index.test.ts:115:23

Read more about building and testing Next.js in contributing.md.

ijjk · 2022-02-16T16:51:11Z

Stats from current PR

Default Build (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	lfades/next.js fix/unexpected-type-import	Change
buildDuration	19.1s	18.9s	-222ms
buildDurationCached	7.1s	7.2s	⚠️ +105ms
nodeModulesSize	368 MB	368 MB	⚠️ +281 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	lfades/next.js fix/unexpected-type-import	Change
/ failed reqs	0	0	✓
/ total time (seconds)	4.123	4.233	⚠️ +0.11
/ avg req/sec	606.33	590.55	⚠️ -15.78
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	2.093	2.148	⚠️ +0.06
/error-in-render avg req/sec	1194.38	1163.91	⚠️ -30.47

Client Bundles (main, webpack, commons)

	vercel/next.js canary	lfades/next.js fix/unexpected-type-import	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	27.9 kB	27.9 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71.5 kB	71.5 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	lfades/next.js fix/unexpected-type-import	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	lfades/next.js fix/unexpected-type-import	Change
_app-HASH.js gzip	1.36 kB	1.36 kB	✓
_error-HASH.js gzip	194 B	194 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	326 B	326 B	✓
dynamic-HASH.js gzip	2.57 kB	2.57 kB	✓
head-HASH.js gzip	350 B	350 B	✓
hooks-HASH.js gzip	919 B	919 B	✓
image-HASH.js gzip	5.01 kB	5.01 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.26 kB	2.26 kB	✓
routerDirect..HASH.js gzip	321 B	321 B	✓
script-HASH.js gzip	383 B	383 B	✓
withRouter-HASH.js gzip	318 B	318 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	14.7 kB	14.7 kB	✓

Client Build Manifests

	vercel/next.js canary	lfades/next.js fix/unexpected-type-import	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes

	vercel/next.js canary	lfades/next.js fix/unexpected-type-import	Change
index.html gzip	532 B	532 B	✓
link.html gzip	545 B	545 B	✓
withRouter.html gzip	526 B	526 B	✓
Overall change	1.6 kB	1.6 kB	✓

Default Build with SWC (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	lfades/next.js fix/unexpected-type-import	Change
buildDuration	23s	23.5s	⚠️ +543ms
buildDurationCached	7.2s	7.3s	⚠️ +42ms
nodeModulesSize	368 MB	368 MB	⚠️ +281 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	lfades/next.js fix/unexpected-type-import	Change
/ failed reqs	0	0	✓
/ total time (seconds)	4.181	4.215	⚠️ +0.03
/ avg req/sec	597.91	593.18	⚠️ -4.73
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	2.09	2.088	0
/error-in-render avg req/sec	1195.96	1197.23	+1.27

Client Bundles (main, webpack, commons)

	vercel/next.js canary	lfades/next.js fix/unexpected-type-import	Change
450.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42.1 kB	42.1 kB	✓
main-HASH.js gzip	27.9 kB	27.9 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	71.6 kB	71.6 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	lfades/next.js fix/unexpected-type-import	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	lfades/next.js fix/unexpected-type-import	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	180 B	180 B	✓
amp-HASH.js gzip	305 B	305 B	✓
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.56 kB	2.56 kB	✓
head-HASH.js gzip	342 B	342 B	✓
hooks-HASH.js gzip	911 B	911 B	✓
image-HASH.js gzip	5.05 kB	5.05 kB	✓
index-HASH.js gzip	256 B	256 B	✓
link-HASH.js gzip	2.28 kB	2.28 kB	✓
routerDirect..HASH.js gzip	314 B	314 B	✓
script-HASH.js gzip	375 B	375 B	✓
withRouter-HASH.js gzip	309 B	309 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	14.7 kB	14.7 kB	✓

Client Build Manifests

	vercel/next.js canary	lfades/next.js fix/unexpected-type-import	Change
_buildManifest.js gzip	458 B	458 B	✓
Overall change	458 B	458 B	✓

Rendered Page Sizes

	vercel/next.js canary	lfades/next.js fix/unexpected-type-import	Change
index.html gzip	533 B	533 B	✓
link.html gzip	547 B	547 B	✓
withRouter.html gzip	528 B	528 B	✓
Overall change	1.61 kB	1.61 kB	✓

Commit: 3f68785

Move type to image component

88fe254

lfades requested review from huozhi, ijjk, shuding, styfle and timneutkens as code owners February 15, 2022 23:59

ijjk added the type: next label Feb 15, 2022

styfle previously approved these changes Feb 16, 2022

View reviewed changes

ijjk requested changes Feb 16, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

Add types/global.d.ts to excludes too

2ea9469

lfades dismissed styfle’s stale review via 2ea9469 February 16, 2022 01:18

Undo global exclude as it's using internally

a6067de

This comment has been minimized.

Sign in to view

Don't add root imports for module augmentations

3f68785

lfades commented Feb 16, 2022

View reviewed changes

lfades requested review from ijjk and styfle February 16, 2022 16:29

styfle approved these changes Feb 16, 2022

View reviewed changes

balazsorban44 merged commit 27affb4 into vercel:canary Feb 19, 2022

github-actions Bot locked as resolved and limited conversation to collaborators Mar 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't import internally from types in `next-env.d.ts`#34394

Don't import internally from types in `next-env.d.ts`#34394
balazsorban44 merged 4 commits intovercel:canaryfrom
lfades:fix/unexpected-type-import

lfades commented Feb 15, 2022

Uh oh!

styfle left a comment

Uh oh!

ijjk left a comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

lfades commented Feb 16, 2022 •

edited

Loading

Uh oh!

This comment has been minimized.

This comment has been minimized.

lfades Feb 16, 2022

Uh oh!

ijjk commented Feb 16, 2022

Uh oh!

ijjk commented Feb 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

lfades commented Feb 15, 2022

Bug

Uh oh!

styfle left a comment

Choose a reason for hiding this comment

Uh oh!

ijjk left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

lfades commented Feb 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

lfades Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

ijjk commented Feb 16, 2022

Failing test suites

Uh oh!

ijjk commented Feb 16, 2022

Stats from current PR

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lfades commented Feb 16, 2022 •

edited

Loading