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

Improve initial setup with new App Router TypeScript project #64826

Merged
merged 9 commits into from
Apr 26, 2024

Conversation

unstubbable
Copy link
Contributor

@unstubbable unstubbable commented Apr 20, 2024

What?

Fixes a couple of issues in the latest Canary regarding the tsconfig.json adjustments that are done after creating a new App Router project with create-next-app. Specifically:

  • Set default TS target to ES2017 (continuation of Improve top level await coverage #64508).
  • Do not warn about disabled strict mode if strict mode was enabled. This is the case after starting the dev command for the first time after having created a new project with create-next-app, because in the tsconfig templates strict is enabled per default.
  • Fix wrongly printed tsconfig changes. See snapshot changes in dc87ebc for details of what was wrong. The bug was introduced in Improve top level await coverage #64508.

Why?

Avoid creating any changes to tsconfig.json in a fresh project on first run, and don't confuse the user with wrong logs.

How?

I fixed the issues in writeConfigurationDefaults(), and added unit tests, as well as adjusted the existing integration tests, to prevent regressions. Additionally, I've created a new project inside of the Next.js monorepo with pnpm create-next-app and ran the dev command inside of this project to verify my fixes.

@ijjk ijjk added create-next-app Related to our CLI tool for quickly starting a new Next.js application. type: next labels Apr 20, 2024
@ijjk
Copy link
Member

ijjk commented Apr 20, 2024

Allow CI Workflow Run

  • approve CI run for commit: 302646a

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

@eps1lon eps1lon added the CI approved Approve running CI for fork label Apr 20, 2024
eps1lon
eps1lon previously approved these changes Apr 20, 2024
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.

Looks good, thank you. CI just needs fixing (or probably just retrying).

@ijjk
Copy link
Member

ijjk commented Apr 20, 2024

Tests Passed

@ijjk
Copy link
Member

ijjk commented Apr 20, 2024

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary unstubbable/next.js improve-app-router-ts-setup Change
buildDuration 18.2s 16.3s N/A
buildDurationCached 10.3s 7.8s N/A
nodeModulesSize 360 MB 360 MB ⚠️ +1.42 kB
nextStartRea..uration (ms) 442ms 439ms N/A
Client Bundles (main, webpack)
vercel/next.js canary unstubbable/next.js improve-app-router-ts-setup Change
139-HASH.js gzip 31.8 kB 31.8 kB N/A
2478adb3-HASH.js gzip 53.5 kB 53.5 kB N/A
4967-HASH.js gzip 5.1 kB 5.1 kB N/A
6701.HASH.js gzip 168 B 168 B
framework-HASH.js gzip 45.2 kB 45.2 kB
main-app-HASH.js gzip 228 B 226 B N/A
main-HASH.js gzip 31.6 kB 31.6 kB N/A
webpack-HASH.js gzip 1.65 kB 1.64 kB N/A
Overall change 45.3 kB 45.3 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary unstubbable/next.js improve-app-router-ts-setup Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary unstubbable/next.js improve-app-router-ts-setup Change
_app-HASH.js gzip 194 B 193 B N/A
_error-HASH.js gzip 192 B 192 B
amp-HASH.js gzip 510 B 510 B
css-HASH.js gzip 343 B 342 B N/A
dynamic-HASH.js gzip 2.51 kB 2.52 kB N/A
edge-ssr-HASH.js gzip 266 B 265 B N/A
head-HASH.js gzip 365 B 362 B N/A
hooks-HASH.js gzip 391 B 390 B N/A
image-HASH.js gzip 4.32 kB 4.32 kB N/A
index-HASH.js gzip 268 B 268 B
link-HASH.js gzip 2.69 kB 2.69 kB N/A
routerDirect..HASH.js gzip 328 B 328 B
script-HASH.js gzip 393 B 395 B N/A
withRouter-HASH.js gzip 323 B 323 B
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 1.73 kB 1.73 kB
Client Build Manifests
vercel/next.js canary unstubbable/next.js improve-app-router-ts-setup Change
_buildManifest.js gzip 484 B 484 B
Overall change 484 B 484 B
Rendered Page Sizes
vercel/next.js canary unstubbable/next.js improve-app-router-ts-setup Change
index.html gzip 528 B 528 B
link.html gzip 540 B 541 B N/A
withRouter.html gzip 523 B 523 B
Overall change 1.05 kB 1.05 kB
Edge SSR bundle Size
vercel/next.js canary unstubbable/next.js improve-app-router-ts-setup Change
edge-ssr.js gzip 108 kB 108 kB N/A
page.js gzip 3.04 kB 3.05 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary unstubbable/next.js improve-app-router-ts-setup Change
middleware-b..fest.js gzip 623 B 623 B
middleware-r..fest.js gzip 155 B 154 B N/A
middleware.js gzip 27.9 kB 27.9 kB N/A
edge-runtime..pack.js gzip 839 B 839 B
Overall change 1.46 kB 1.46 kB
Next Runtimes
vercel/next.js canary unstubbable/next.js improve-app-router-ts-setup Change
app-page-exp...dev.js gzip 171 kB 171 kB
app-page-exp..prod.js gzip 98.4 kB 98.4 kB
app-page-tur..prod.js gzip 99.9 kB 99.9 kB
app-page-tur..prod.js gzip 94.2 kB 94.2 kB
app-page.run...dev.js gzip 157 kB 157 kB
app-page.run..prod.js gzip 93 kB 93 kB
app-route-ex...dev.js gzip 21.4 kB 21.4 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.3 kB 21.3 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 65.3 kB 65.3 kB
Overall change 975 kB 975 kB
build cache Overall increase ⚠️
vercel/next.js canary unstubbable/next.js improve-app-router-ts-setup Change
0.pack gzip 1.61 MB 1.61 MB ⚠️ +158 B
index.pack gzip 112 kB 112 kB N/A
Overall change 1.61 MB 1.61 MB ⚠️ +158 B
Diff details
Diff for main-HASH.js

Diff too large to display

Commit: a4e8c97

@unstubbable unstubbable force-pushed the improve-app-router-ts-setup branch 3 times, most recently from e7e0a1f to 2f6421c Compare April 22, 2024 13:05
@ryan-nauman
Copy link
Contributor

fyi this slightly relates to #61413 as well

@eps1lon
Copy link
Member

eps1lon commented Apr 23, 2024

fyi this slightly relates to #61413 as well

@ryan-nauman How are these related?

@ryan-nauman
Copy link
Contributor

ryan-nauman commented Apr 23, 2024

@eps1lon both add tests for writeConfigurationDefaults.ts

  • the location in this PR is packages/next/src/lib/typescript/writeConfigurationDefaults.test.ts
  • the location in that PR is test/unit/write-configuration-defaults.test.ts

it could be confusing to have them in both places

@unstubbable
Copy link
Contributor Author

unstubbable commented Apr 23, 2024

@eps1lon both have tests for writeConfigurationDefaults.ts

  • the location in this PR is packages/next/src/lib/typescript/writeConfigurationDefaults.test.ts
  • the location in that PR is test/unit/write-configuration-defaults.test.ts

it could be confusing to have them in both places

Yeah, I need to merge those now, after rebasing. I'm not sure about the history of test/unit, but it seems that nowadays unit tests are often colocated with their implementation. The unit test in test/unit is also importing its implementation from next/dist, and I don't see a moduleNameMapper, so does this require a build when the implementation is changed? (Can't test it myself right now, but might be able to do so this evening, CET.)

@ryan-nauman
Copy link
Contributor

so does this require a build when the implementation is changed?

yes. same as test/unit/write-app-declarations.test.ts

i followed the guide in https://github.com/vercel/next.js/blob/canary/contributing/core/testing.md. i have one console with pnpm dev running. as i make changes to the implementation i run pnpm testonly-dev .../pnpm testonly-start ...

@unstubbable
Copy link
Contributor Author

unstubbable commented Apr 26, 2024

@eps1lon I've now merged the two unit test files for writeConfigurationDefaults, and also added integration test expectations to ensure that there are no git changes in a newly created project with TS.

@unstubbable unstubbable requested a review from a team as a code owner April 26, 2024 09:49
@unstubbable unstubbable removed the request for review from a team April 26, 2024 09:49
@unstubbable unstubbable marked this pull request as draft April 26, 2024 09:55
@unstubbable
Copy link
Contributor Author

unstubbable commented Apr 26, 2024

Could you also update all the test fixtures with a tsconfig? Otherwise running tests in those adds unrelated changes in git.

@eps1lon Do you mean "running tests" as in pnpm next dev test/e2e/app-dir/modularizeimports, for example? So I should update all tsconfigs in examples and in test?

@ijjk ijjk added the examples Issue/PR related to examples label Apr 26, 2024
@eps1lon
Copy link
Member

eps1lon commented Apr 26, 2024

So I should update all tsconfigs in examples and in test?

At least test, yes. Sometimes we run the Next cli directly on those packages which would now change the tsconfig.

@unstubbable
Copy link
Contributor Author

So I should update all tsconfigs in examples and in test?

At least test, yes. Sometimes we run the Next cli directly on those packages which would now change the tsconfig.

Alright, done.

Just to make sure we're having the same understanding regarding the statement "which would now change the tsconfig":
This PR does not change anything about that. Potentially changing the tsconfig when running next dev or next lint, is already happening on canary. That this currently happens for newly created projects was caused by #64508.

@unstubbable unstubbable marked this pull request as ready for review April 26, 2024 12:47
Continuation of vercel#64508

This avoids needing to update a freshly created tsconfig.json when first
running the `dev` command.
This is the case after starting the `dev` command for the first time,
after having created a new project with `create-next-app`.
This should avoid getting unrelated git changes when running `next dev`
in any of these projects.

Commands used to update the files:

```sh
find test -name tsconfig.json -exec dirname {} \; | while read dir; do pnpm next lint "$dir"; done
pnpm format
```
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.

I'll babysit CI to get this in. Great work, thank you!

@eps1lon eps1lon merged commit 3438b39 into vercel:canary Apr 26, 2024
74 checks passed
@unstubbable unstubbable deleted the improve-app-router-ts-setup branch April 26, 2024 16:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI approved Approve running CI for fork create-next-app Related to our CLI tool for quickly starting a new Next.js application. examples Issue/PR related to examples locked tests type: next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants