Skip to content
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.

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

Upgrade typescript to 5.3 #64043

Merged
merged 14 commits into from
Apr 17, 2024
Merged

Upgrade typescript to 5.3 #64043

merged 14 commits into from
Apr 17, 2024

Conversation

huozhi
Copy link
Member

@huozhi huozhi commented Apr 3, 2024

Closes NEXT-2997

@ijjk ijjk added create-next-app Related to our CLI tool for quickly starting a new Next.js application. created-by: Next.js team PRs by the Next.js team. type: next labels Apr 3, 2024
Copy link

socket-security bot commented Apr 3, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@types/node@20.12.3 None +1 2.09 MB types
npm/@types/react-dom@18.2.23 None 0 34.9 kB types
npm/@types/react-is@18.2.4 None 0 5.67 kB types
npm/@types/react@18.2.74 None +2 1.65 MB types
npm/typescript@5.3.3 None 0 32 MB typescript-bot

🚮 Removed packages: npm/@types/node@20.2.5, npm/@types/react-dom@18.2.15, npm/@types/react-is@17.0.3, npm/@types/react@18.2.37, npm/typescript@5.2.2

View full report↗︎

@ijjk
Copy link
Member

ijjk commented Apr 3, 2024

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js upgrade-ts-5-4 Change
buildDuration 14.2s 14.3s N/A
buildDurationCached 8.8s 6.7s N/A
nodeModulesSize 199 MB 199 MB ⚠️ +10.4 kB
nextStartRea..uration (ms) 381ms 386ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js upgrade-ts-5-4 Change
2453-HASH.js gzip 31.5 kB 31.5 kB N/A
3304.HASH.js gzip 181 B 181 B
3f784ff6-HASH.js gzip 53.7 kB 53.7 kB
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 242 B 242 B
main-HASH.js gzip 29.6 kB 29.7 kB N/A
webpack-HASH.js gzip 1.68 kB 1.68 kB N/A
Overall change 99.3 kB 99.3 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js upgrade-ts-5-4 Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js upgrade-ts-5-4 Change
_app-HASH.js gzip 196 B 197 B N/A
_error-HASH.js gzip 184 B 184 B
amp-HASH.js gzip 505 B 505 B
css-HASH.js gzip 324 B 325 B N/A
dynamic-HASH.js gzip 2.5 kB 2.5 kB N/A
edge-ssr-HASH.js gzip 258 B 258 B
head-HASH.js gzip 352 B 352 B
hooks-HASH.js gzip 370 B 371 B N/A
image-HASH.js gzip 4.27 kB 4.27 kB
index-HASH.js gzip 259 B 259 B
link-HASH.js gzip 2.67 kB 2.67 kB N/A
routerDirect..HASH.js gzip 314 B 312 B N/A
script-HASH.js gzip 386 B 386 B
withRouter-HASH.js gzip 309 B 309 B
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 6.63 kB 6.63 kB
Client Build Manifests
vercel/next.js canary vercel/next.js upgrade-ts-5-4 Change
_buildManifest.js gzip 483 B 485 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js upgrade-ts-5-4 Change
index.html gzip 528 B 527 B N/A
link.html gzip 541 B 540 B N/A
withRouter.html gzip 524 B 522 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js upgrade-ts-5-4 Change
edge-ssr.js gzip 94.4 kB 94.4 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 vercel/next.js upgrade-ts-5-4 Change
middleware-b..fest.js gzip 624 B 626 B N/A
middleware-r..fest.js gzip 155 B 156 B N/A
middleware.js gzip 25.5 kB 25.5 kB N/A
edge-runtime..pack.js gzip 839 B 839 B
Overall change 839 B 839 B
Next Runtimes
vercel/next.js canary vercel/next.js upgrade-ts-5-4 Change
app-page-exp...dev.js gzip 171 kB 171 kB
app-page-exp..prod.js gzip 97.5 kB 97.5 kB
app-page-tur..prod.js gzip 99.2 kB 99.2 kB
app-page-tur..prod.js gzip 93.5 kB 93.5 kB
app-page.run...dev.js gzip 145 kB 145 kB
app-page.run..prod.js gzip 92 kB 92 kB
app-route-ex...dev.js gzip 21.5 kB 21.5 kB
app-route-ex..prod.js gzip 15.2 kB 15.2 kB
app-route-tu..prod.js gzip 15.2 kB 15.2 kB
app-route-tu..prod.js gzip 14.9 kB 14.9 kB
app-route.ru...dev.js gzip 21.1 kB 21.1 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.5 kB 51.5 kB
Overall change 945 kB 945 kB
build cache Overall increase ⚠️
vercel/next.js canary vercel/next.js upgrade-ts-5-4 Change
0.pack gzip 1.59 MB 1.59 MB ⚠️ +1.21 kB
index.pack gzip 107 kB 106 kB N/A
Overall change 1.59 MB 1.59 MB ⚠️ +1.21 kB
Diff details
Diff for middleware.js

Diff too large to display

Commit: 962f318

@huozhi huozhi changed the title [WIP] test ts [WIP] ts 5.4 Apr 4, 2024
@huozhi huozhi changed the title [WIP] ts 5.4 [WIP] upgrade ts Apr 4, 2024
@eps1lon
Copy link
Member

eps1lon commented Apr 8, 2024

Looks like the CI error is legit. I'll check it out.

@@ -1354,7 +1349,7 @@ export async function renderToHTMLImpl(
(initialStream: ReactReadableStream, suffix?: string) => {
return continueFizzStream(initialStream, {
suffix,
inlinedDataStream: serverComponentsInlinedTransformStream?.readable,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

serverComponentsInlinedTransformStream is always null so TypeScript complained about unreachable code since serverComponentsInlinedTransformStream?.readable is equivalent to undefined.

@ijjk
Copy link
Member

ijjk commented Apr 14, 2024

Failing test suites

Commit: 793e9a9

TURBOPACK=1 pnpm test test/integration/create-next-app/templates/pages.test.ts (turbopack)

  • create-next-app --no-app (Pages Router) > should create JavaScript project with --js flag
  • create-next-app --no-app (Pages Router) > should create TypeScript project with --ts flag
  • create-next-app --no-app (Pages Router) > should create project inside "src" directory with --src-dir flag
  • create-next-app --no-app (Pages Router) > should create TailwindCSS project with --tailwind flag
Expand output

● create-next-app --no-app (Pages Router) › should create JavaScript project with --js flag

FetchError: request to http://localhost:41419/ failed, reason: connect ECONNREFUSED 127.0.0.1:41419

  at ClientRequest.<anonymous> (../node_modules/.pnpm/node-fetch@2.6.7/node_modules/node-fetch/lib/index.js:1491:11)

● create-next-app --no-app (Pages Router) › should create TypeScript project with --ts flag

FetchError: request to http://localhost:41891/ failed, reason: connect ECONNREFUSED 127.0.0.1:41891

  at ClientRequest.<anonymous> (../node_modules/.pnpm/node-fetch@2.6.7/node_modules/node-fetch/lib/index.js:1491:11)

● create-next-app --no-app (Pages Router) › should create project inside "src" directory with --src-dir flag

FetchError: request to http://localhost:41219/ failed, reason: connect ECONNREFUSED 127.0.0.1:41219

  at ClientRequest.<anonymous> (../node_modules/.pnpm/node-fetch@2.6.7/node_modules/node-fetch/lib/index.js:1491:11)

● create-next-app --no-app (Pages Router) › should create TailwindCSS project with --tailwind flag

FetchError: request to http://localhost:46627/ failed, reason: connect ECONNREFUSED 127.0.0.1:46627

  at ClientRequest.<anonymous> (../node_modules/.pnpm/node-fetch@2.6.7/node_modules/node-fetch/lib/index.js:1491:11)

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

@ijjk
Copy link
Member

ijjk commented Apr 14, 2024

Tests Passed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading #59776, it seems like the root issue was resolve with #64117

The root tsconfig already includes the test files so it's unclear what the point of this project is.

Copy link
Member

@eps1lon eps1lon Apr 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeScript didn't care to reference misc.d.ts in reachable declaration files for some reason. Now it does so we have two options:

  1. Force the old behavior by not referencing misc.d.ts at all
  2. Get rid of the two-world type-checking where internally we emit declarations with misc.d.ts but then publicly rely on compiled.d.ts existing in the program

Point 2 is quite involved since it requires having either the same types for internal and public or at least public types that are compatible with internal types. Right now, the public types are not compatible with the internal types (e.g. you can't redeclare a value even though its type is any). We'd have to inline Webpack types which is quite involved (mostly because there's no tool to bundle types, see microsoft/TypeScript#4433).

In this PR I choose 1. by rewriting references to $$compiled.internal.d.ts (the old misc.d.ts) to compiled.d.ts. This has the benefit of more clearly communicating what misc.d.ts and compiled.d.ts actually represented.

That the rewrite is sound (or at least as sound as prior typings were) is ensured by the new typechecking tests from #64478 and all the existing tests including type-checking.

@eps1lon eps1lon force-pushed the upgrade-ts-5-4 branch 2 times, most recently from 24914f3 to f730162 Compare April 15, 2024 15:54
@eps1lon eps1lon self-assigned this Apr 16, 2024
Copy link
Contributor

github-actions bot commented Apr 16, 2024

All broken links are now fixed, thank you!

@eps1lon eps1lon force-pushed the upgrade-ts-5-4 branch 3 times, most recently from 086a398 to 333cb78 Compare April 16, 2024 18:37
@@ -1,5 +1,4 @@
/// <reference types="./types/global" />
/// <reference types="./types/compiled" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automatically referenced in the files that are relevant.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a new safety measure in case somebody imports from next/types without a type modifier. Such an import may not be elided by during transform which may result in bundler errors because next/types.js doesn't exist.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved into packages/next/types/$$compiled.internal.d.ts

eps1lon and others added 9 commits April 17, 2024 09:51
Fixes
 error TS2339: Property 'error' does not exist on type 'SafeParseReturnType<NextConfig, NextConfig>'.
  Property 'error' does not exist on type 'SafeParseSuccess<NextConfig>'.

1021         const [errorMessages, shouldExit] = normalizeZodErrors(state.error)
Combines misc.d.ts and webpack.d.ts into `$$compiled.internal.d.ts`.

`$$compiled.internal.d.ts` represents type declarations necessary to type the implementation while `compiled.d.ts`
represents declarations necessary to type the emitted declarations.

This internal/public split is really just a typechecking perf improvement.

We should just be able to include both public and internal ambient declarations in the declaratione emit program
and then just test the internal one isn't referenced.
Removes a bunch of `@ts-ignore` and is more in line with our existing entrypoints.
Required to entangle the public/internal types plit
5x more imports of dist than src

Importing from src doesn't typecheck.
@huozhi huozhi changed the title [WIP] upgrade ts Upgrade typescript Apr 17, 2024
@huozhi huozhi marked this pull request as ready for review April 17, 2024 09:30
Copy link

vercel bot commented Apr 17, 2024

Notifying the following users due to files changed in this PR based on this repo's notify modifiers:

@timneutkens, @ijjk, @shuding:

packages/next/src/server/config.ts

@huozhi huozhi changed the title Upgrade typescript Upgrade typescript to 5.3 Apr 17, 2024
Copy link
Member Author

@huozhi huozhi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of changes are made by @eps1lon , LGTM

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

obama-awards-obama-a-medal.gif

@@ -5,7 +5,7 @@ import type {
Visitor,
NodePath,
} from 'next/dist/compiled/babel/core'
import type { PageConfig } from 'next/types'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a public api that we are going to break? I dont know if next/types is something the user would expect to import from.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be the same content. next/types used to resolve to next/types/index.d.ts. Now it resolves to next/types.d.ts which just reexports next/dist/types.d.ts. And since I just moved everything from next/types/index.d.ts into next/src/types.ts, the content should also be the same.

It's really just reorganizing files so that the program using the root tsconfig doesn't include Next.js source files.

@huozhi huozhi merged commit 1e3a1cb into canary Apr 17, 2024
82 checks passed
@huozhi huozhi deleted the upgrade-ts-5-4 branch April 17, 2024 16:35
@github-actions github-actions bot added the locked label May 2, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
create-next-app Related to our CLI tool for quickly starting a new Next.js application. created-by: Next.js team PRs by the Next.js team. locked type: next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants