fix: improve tsconfig extends checks #61413

ryan-nauman · 2024-01-30T21:48:09Z

What?

This pr improves tsconfig modifications when appdir is used. Specifically:

tsconfig isn't changed to add strictNullChecks when the extends configuration satisfies it
tsconfig include that is missing app directory types (.next/types/**/*.ts) is safely changed to include them as well as the inherited include's

Why?

The extends configuration isn't accounted for in some of the flag checks.

Fixes #61409

ryan-nauman · 2024-01-30T21:49:08Z

packages/next/src/lib/typescript/writeConfigurationDefaults.ts

    }

-    // If `strict` is set to `false` or `strictNullChecks` is set to `false`,
+    // If `strict` is set to `false` and `strictNullChecks` is set to `false`,


updated this comment to reflect the checks

ryan-nauman · 2024-01-30T21:50:56Z

packages/next/src/lib/typescript/writeConfigurationDefaults.ts

+      !tsOptions.strict &&
+      !('strictNullChecks' in tsOptions)


favoring tsOptions which includes the configs from the extends target

ryan-nauman · 2024-02-06T13:53:01Z

@timneutkens / @ijjk mind giving this one a look?

ryan-nauman · 2024-02-27T11:39:53Z

can i get a review on this?

timneutkens

Hey, sorry for not getting back to you earlier. The reason this hasn't been reviewed is that it is missing tests for the changes. Can you add tests to cover the changes you're making. Thanks!

test/unit/write-app-declarations.test.ts

ryan-nauman · 2024-04-18T10:51:36Z

Hey, sorry for not getting back to you earlier. The reason this hasn't been reviewed is that it is missing tests for the changes. Can you add tests to cover the changes you're making. Thanks!

sure thing. this file didn't have any tests before. i added some for my changes and followed the pattern of test/unit/write-app-declarations.test.ts.

test/unit/write-app-declarations.test.ts

eps1lon · 2024-04-18T13:25:27Z

test/unit/write-configuration-defaults.test.ts

+    await fs.remove(tsconfigBaseFile)
+  })
+
+  it('should support empty includes when base provides it', async () => {


Isn't the issue now that the resulting tsconfig will have an incomplete include because next-env.d.ts is missing? Would be best if the test would assert on the final include just like the other test does for strictNullChecks

thanks for the feedback!

next-env.d.ts being included or not is outside the scope of this change. that path only occurs when there is no include at all. details below.

i'm happy to extend the expects to check for nextAppTypes. i'm also happy to make the test suite for this file more robust but i was thinking it'd be best to start small on my first PR here and continue in a follow-up(s).

for example, a tsconfig.json on canary without this PR like this

{ "compilerOptions": { "plugins": [ { "name": "next" } ] } }

is transformed into

{ "compilerOptions": { ... }, "include": [ "**/*.ts", "**/*.tsx", ".next/types/**/*.ts" ], ... }

but a tsconfig.json & tsconfig.base.json like this

// tsconfig.json { "extends": "./tsconfig.base.json", "compilerOptions": { "plugins": [ { "name": "next" } ] } } // tsconfig.base.json { "include": ["**/*.ts", "**/*.tsx"] }

throws

TypeError: Cannot read properties of undefined (reading 'push') at writeConfigurationDefaults (/tsconfig-errors-reproduction-app/node_modules/.pnpm/[email protected][email protected][email protected]/node_modules/next/dist/lib/typescript/writeConfigurationDefaults.js:229:30)

Throwing seems better to me than producing an incorrect include. We should do this in this PR so that we can revert easily.

i want to make sure we are on the same page. in this PR tsconfig.json & tsconfig.base.json

// tsconfig.json { "extends": "./tsconfig.base.json", "compilerOptions": { "plugins": [ { "name": "next" } ] } } // tsconfig.base.json { "include": ["**/*.ts", "**/*.tsx"] }

produce's a modified tsconfig.json as this

{ "extends": "./tsconfig.base.json", "compilerOptions": { "plugins": [ { "name": "next" } ], "target": "ES2017", "lib": [ "dom", "dom.iterable", "esnext" ], "allowJs": true, "skipLibCheck": true, "strict": false, "noEmit": true, "incremental": true, "module": "esnext", "esModuleInterop": true, "moduleResolution": "node", "resolveJsonModule": true, "isolatedModules": true, "jsx": "preserve", "strictNullChecks": true }, "include": [ ".next/types/**/*.ts" ], "exclude": [ "node_modules" ] }

the include from the tsconfig.json produced will make the base includes disregarded. which option as the include are you looking for? we could

reach into the base and merge the two arrays and produce ["**/*.ts", "**/*.tsx", ".next/types/**/*.ts"] in tsconfig.json

manipulate tsconfig.base.json and make include be ["**/*.ts", "**/*.tsx", ".next/types/**/*.ts"]

something else - i'm unsure if you are hinting at next-env.d.ts or not

i think (hope?) most users with extends understand the inheritance which is why i am okay with my PR's output. option 2 becomes slippery since you can chain extends across multiple files (a extends b extends c).

Option 1 seems right. Even if the next specific includes may be redundant, they're still important.

sounds good to me - updated

test/unit/write-configuration-defaults.test.ts

Co-authored-by: Sebastian Silbermann <[email protected]>

eps1lon

This looks good now. Thank you for sticking with it. Could you expand in the PR description on what changed specifically? a "improved tsconfig handling" is too ambigious for somebody who wants to learn about the change by scanning the PR description.

ryan-nauman · 2024-04-19T19:43:47Z

This looks good now. Thank you for sticking with it. Could you expand in the PR description on what changed specifically? a "improved tsconfig handling" is too ambigious for somebody who wants to learn about the change by scanning the PR description.

updated. thanks for all the feedback. i'm happy to get this in and continue to expand tweaks to this file & the test suite in a follow up

…canary

ryan-nauman · 2024-04-21T12:57:35Z

updated

looks like there's some integrations test that i'll have to update

https://github.com/vercel/next.js/actions/runs/8758796249/job/24057693758?pr=61413#step:27:308

ijjk · 2024-04-21T13:02:34Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	ryan-nauman/next.js canary	Change
buildDuration	14.9s	15s	N/A
buildDurationCached	8.2s	6.9s	N/A
nodeModulesSize	199 MB	199 MB	⚠️ +5.32 kB
nextStartRea..uration (ms)	408ms	401ms	N/A

Client Bundles (main, webpack)

	vercel/next.js canary	ryan-nauman/next.js canary	Change
2453-HASH.js gzip	31.5 kB	31.5 kB	N/A
3304.HASH.js gzip	169 B	169 B	✓
3f784ff6-HASH.js gzip	53.7 kB	53.7 kB	N/A
8299-HASH.js gzip	5.1 kB	5.1 kB	N/A
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	226 B	227 B	N/A
main-HASH.js gzip	29.6 kB	29.7 kB	N/A
webpack-HASH.js gzip	1.64 kB	1.65 kB	N/A
Overall change	45.4 kB	45.4 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	ryan-nauman/next.js canary	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	ryan-nauman/next.js canary	Change
_app-HASH.js gzip	193 B	194 B	N/A
_error-HASH.js gzip	193 B	191 B	N/A
amp-HASH.js gzip	511 B	511 B	✓
css-HASH.js gzip	342 B	343 B	N/A
dynamic-HASH.js gzip	2.51 kB	2.51 kB	N/A
edge-ssr-HASH.js gzip	265 B	265 B	✓
head-HASH.js gzip	365 B	364 B	N/A
hooks-HASH.js gzip	389 B	391 B	N/A
image-HASH.js gzip	4.28 kB	4.28 kB	N/A
index-HASH.js gzip	269 B	268 B	N/A
link-HASH.js gzip	2.68 kB	2.69 kB	N/A
routerDirect..HASH.js gzip	328 B	326 B	N/A
script-HASH.js gzip	395 B	397 B	N/A
withRouter-HASH.js gzip	323 B	323 B	✓
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	1.21 kB	1.21 kB	✓

Client Build Manifests

	vercel/next.js canary	ryan-nauman/next.js canary	Change
_buildManifest.js gzip	483 B	485 B	N/A
Overall change	0 B	0 B	✓

Rendered Page Sizes

	vercel/next.js canary	ryan-nauman/next.js canary	Change
index.html gzip	529 B	530 B	N/A
link.html gzip	541 B	542 B	N/A
withRouter.html gzip	524 B	523 B	N/A
Overall change	0 B	0 B	✓

Edge SSR bundle Size

	vercel/next.js canary	ryan-nauman/next.js canary	Change
edge-ssr.js gzip	94.5 kB	94.5 kB	N/A
page.js gzip	3.05 kB	3.05 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	ryan-nauman/next.js canary	Change
middleware-b..fest.js gzip	623 B	625 B	N/A
middleware-r..fest.js gzip	155 B	156 B	N/A
middleware.js gzip	25.6 kB	25.6 kB	N/A
edge-runtime..pack.js gzip	839 B	839 B	✓
Overall change	839 B	839 B	✓

Next Runtimes

	vercel/next.js canary	ryan-nauman/next.js canary	Change
app-page-exp...dev.js gzip	171 kB	171 kB	✓
app-page-exp..prod.js gzip	97.6 kB	97.6 kB	✓
app-page-tur..prod.js gzip	99.4 kB	99.4 kB	✓
app-page-tur..prod.js gzip	93.6 kB	93.6 kB	✓
app-page.run...dev.js gzip	145 kB	145 kB	✓
app-page.run..prod.js gzip	92.1 kB	92.1 kB	✓
app-route-ex...dev.js gzip	21.5 kB	21.5 kB	✓
app-route-ex..prod.js gzip	15.1 kB	15.1 kB	✓
app-route-tu..prod.js gzip	15.1 kB	15.1 kB	✓
app-route-tu..prod.js gzip	14.9 kB	14.9 kB	✓
app-route.ru...dev.js gzip	21.2 kB	21.2 kB	✓
app-route.ru..prod.js gzip	14.9 kB	14.9 kB	✓
pages-api-tu..prod.js gzip	9.55 kB	9.55 kB	✓
pages-api.ru...dev.js gzip	9.82 kB	9.82 kB	✓
pages-api.ru..prod.js gzip	9.55 kB	9.55 kB	✓
pages-turbo...prod.js gzip	21.4 kB	21.4 kB	✓
pages.runtim...dev.js gzip	22.1 kB	22.1 kB	✓
pages.runtim..prod.js gzip	21.4 kB	21.4 kB	✓
server.runti..prod.js gzip	51.6 kB	51.6 kB	✓
Overall change	946 kB	946 kB	✓

build cache

	vercel/next.js canary	ryan-nauman/next.js canary	Change
0.pack gzip	1.6 MB	1.59 MB	N/A
index.pack gzip	107 kB	107 kB	N/A
Overall change	0 B	0 B	✓

Diff details

Diff for middleware.js

Diff too large to display

Diff for image-HASH.js

@@ -1,7 +1,7 @@
 (self["webpackChunk_N_E"] = self["webpackChunk_N_E"] || []).push([
   [8358],
   {
-    /***/ 1552: /***/ (
+    /***/ 4070: /***/ (
       __unused_webpack_module,
       __unused_webpack_exports,
       __webpack_require__
@@ -9,7 +9,7 @@
       (window.__NEXT_P = window.__NEXT_P || []).push([
         "/image",
         function () {
-          return __webpack_require__(5237);
+          return __webpack_require__(396);
         },
       ]);
       if (false) {
@@ -18,7 +18,7 @@
       /***/
     },
 
-    /***/ 2016: /***/ (module, exports, __webpack_require__) => {
+    /***/ 8490: /***/ (module, exports, __webpack_require__) => {
       "use strict";
       /* __next_internal_client_entry_do_not_use__  cjs */
       Object.defineProperty(exports, "__esModule", {
@@ -40,15 +40,15 @@
         __webpack_require__(422)
       );
       const _head = /*#__PURE__*/ _interop_require_default._(
-        __webpack_require__(6074)
+        __webpack_require__(2457)
       );
-      const _getimgprops = __webpack_require__(9571);
-      const _imageconfig = __webpack_require__(6567);
-      const _imageconfigcontextsharedruntime = __webpack_require__(419);
-      const _warnonce = __webpack_require__(4486);
-      const _routercontextsharedruntime = __webpack_require__(162);
+      const _getimgprops = __webpack_require__(7932);
+      const _imageconfig = __webpack_require__(5706);
+      const _imageconfigcontextsharedruntime = __webpack_require__(9483);
+      const _warnonce = __webpack_require__(9035);
+      const _routercontextsharedruntime = __webpack_require__(4829);
       const _imageloader = /*#__PURE__*/ _interop_require_default._(
-        __webpack_require__(6996)
+        __webpack_require__(7240)
       );
       // This is replaced by webpack define plugin
       const configEnv = {
@@ -379,7 +379,7 @@
       /***/
     },
 
-    /***/ 9571: /***/ (
+    /***/ 7932: /***/ (
       __unused_webpack_module,
       exports,
       __webpack_require__
@@ -395,9 +395,9 @@
           return getImgProps;
         },
       });
-      const _warnonce = __webpack_require__(4486);
-      const _imageblursvg = __webpack_require__(133);
-      const _imageconfig = __webpack_require__(6567);
+      const _warnonce = __webpack_require__(9035);
+      const _imageblursvg = __webpack_require__(2642);
+      const _imageconfig = __webpack_require__(5706);
       const VALID_LOADING_VALUES =
         /* unused pure expression or super */ null && [
           "lazy",
@@ -772,7 +772,7 @@
       /***/
     },
 
-    /***/ 133: /***/ (__unused_webpack_module, exports) => {
+    /***/ 2642: /***/ (__unused_webpack_module, exports) => {
       "use strict";
       /**
        * A shared function, used on both client and server, to generate a SVG blur placeholder.
@@ -827,7 +827,7 @@
       /***/
     },
 
-    /***/ 4085: /***/ (
+    /***/ 503: /***/ (
       __unused_webpack_module,
       exports,
       __webpack_require__
@@ -854,10 +854,10 @@
         },
       });
       const _interop_require_default = __webpack_require__(2430);
-      const _getimgprops = __webpack_require__(9571);
-      const _imagecomponent = __webpack_require__(2016);
+      const _getimgprops = __webpack_require__(7932);
+      const _imagecomponent = __webpack_require__(8490);
       const _imageloader = /*#__PURE__*/ _interop_require_default._(
-        __webpack_require__(6996)
+        __webpack_require__(7240)
       );
       function getImageProps(imgProps) {
         const { props } = (0, _getimgprops.getImgProps)(imgProps, {
@@ -889,7 +889,7 @@
       /***/
     },
 
-    /***/ 6996: /***/ (__unused_webpack_module, exports) => {
+    /***/ 7240: /***/ (__unused_webpack_module, exports) => {
       "use strict";
 
       Object.defineProperty(exports, "__esModule", {
@@ -924,7 +924,7 @@
       /***/
     },
 
-    /***/ 5237: /***/ (
+    /***/ 396: /***/ (
       __unused_webpack_module,
       __webpack_exports__,
       __webpack_require__
@@ -941,8 +941,8 @@
 
       // EXTERNAL MODULE: ./node_modules/.pnpm/[email protected]/node_modules/react/jsx-runtime.js
       var jsx_runtime = __webpack_require__(1527);
-      // EXTERNAL MODULE: ./node_modules/.pnpm/[email protected][email protected]/node_modules/next/image.js
-      var next_image = __webpack_require__(1577);
+      // EXTERNAL MODULE: ./node_modules/.pnpm/[email protected][email protected]/node_modules/next/image.js
+      var next_image = __webpack_require__(73);
       var image_default = /*#__PURE__*/ __webpack_require__.n(next_image); // CONCATENATED MODULE: ./pages/nextjs.png
       /* harmony default export */ const nextjs = {
         src: "/_next/static/media/nextjs.cae0b805.png",
@@ -972,12 +972,8 @@
       /***/
     },
 
-    /***/ 1577: /***/ (
-      module,
-      __unused_webpack_exports,
-      __webpack_require__
-    ) => {
-      module.exports = __webpack_require__(4085);
+    /***/ 73: /***/ (module, __unused_webpack_exports, __webpack_require__) => {
+      module.exports = __webpack_require__(503);
 
       /***/
     },
@@ -987,7 +983,7 @@
     /******/ var __webpack_exec__ = (moduleId) =>
       __webpack_require__((__webpack_require__.s = moduleId));
     /******/ __webpack_require__.O(0, [2888, 9774, 179], () =>
-      __webpack_exec__(1552)
+      __webpack_exec__(4070)
     );
     /******/ var __webpack_exports__ = __webpack_require__.O();
     /******/ _N_E = __webpack_exports__;

Diff for 2453-HASH.js

Diff too large to display

Diff for main-HASH.js

Diff too large to display

Commit: cf41528

Stale review since tests are now added

ryan-nauman requested review from feedthejim, huozhi, ijjk, shuding, timneutkens, wyattjoh and ztanner as code owners January 30, 2024 21:48

ijjk added the type: next label Jan 30, 2024

ryan-nauman commented Jan 30, 2024

View reviewed changes

timneutkens previously requested changes Apr 17, 2024

View reviewed changes

ryan-nauman closed this Apr 18, 2024

ryan-nauman force-pushed the canary branch from 00c434f to 134a59d Compare April 18, 2024 10:35

fix configuration defaults

6fed2e1

ryan-nauman reopened this Apr 18, 2024

ijjk added the tests label Apr 18, 2024

ryan-nauman commented Apr 18, 2024

View reviewed changes

test/unit/write-app-declarations.test.ts Outdated Show resolved Hide resolved

ryan-nauman requested a review from timneutkens April 18, 2024 10:51

eps1lon reviewed Apr 18, 2024

View reviewed changes

ryan-nauman force-pushed the canary branch from cab0028 to 6fed2e1 Compare April 18, 2024 18:07

ryan-nauman and others added 2 commits April 19, 2024 13:42

safely preserve include

0155479

Merge branch 'canary' into canary

2976020

eps1lon reviewed Apr 19, 2024

View reviewed changes

test/unit/write-configuration-defaults.test.ts Outdated Show resolved Hide resolved

ryan-nauman and others added 2 commits April 19, 2024 13:59

test readability

7288fcd

Co-authored-by: Sebastian Silbermann <[email protected]>

add snapshot

13400f4

eps1lon approved these changes Apr 19, 2024

View reviewed changes

Merge branch 'canary' into canary

0d82aa5

eps1lon added the CI approved Approve running CI for fork label Apr 20, 2024

ryan-nauman added 2 commits April 21, 2024 08:55

lint fix

bd1f3cb

Merge branch 'canary' of https://github.com/ryan-nauman/next.js into …

829d1ca

…canary

ryan-nauman and others added 2 commits April 21, 2024 19:22

update integration snapshots

30e441d

Merge branch 'canary' into canary

cf41528

eps1lon approved these changes Apr 22, 2024

View reviewed changes

eps1lon enabled auto-merge (squash) April 22, 2024 14:05

ryan-nauman mentioned this pull request Apr 22, 2024

Improve initial setup with new App Router TypeScript project #64826

Merged

eps1lon merged commit c09ce24 into vercel:canary Apr 23, 2024

github-actions bot added the locked label May 7, 2024

github-actions bot locked as resolved and limited conversation to collaborators May 7, 2024

Uh oh!

fix: improve tsconfig extends checks #61413

fix: improve tsconfig extends checks #61413

Uh oh!

Conversation

ryan-nauman commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What?

Why?

Uh oh!

ryan-nauman Jan 30, 2024

Choose a reason for hiding this comment

Uh oh!

ryan-nauman Jan 30, 2024

Choose a reason for hiding this comment

Uh oh!

ryan-nauman commented Feb 6, 2024

Uh oh!

ryan-nauman commented Feb 27, 2024

Uh oh!

timneutkens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryan-nauman commented Apr 18, 2024

Uh oh!

Uh oh!

eps1lon Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

ryan-nauman Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

eps1lon Apr 18, 2024

Choose a reason for hiding this comment

Uh oh!

ryan-nauman Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eps1lon Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

ryan-nauman Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eps1lon left a comment

Choose a reason for hiding this comment

Uh oh!

ryan-nauman commented Apr 19, 2024

Uh oh!

ryan-nauman commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ijjk commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Stats from current PR

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ryan-nauman commented Jan 30, 2024 •

edited

Loading

ryan-nauman Apr 18, 2024 •

edited

Loading

ryan-nauman commented Apr 21, 2024 •

edited

Loading

ijjk commented Apr 21, 2024 •

edited

Loading