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

feat: allow module: Preserve tsconfig option #64110

Merged

Conversation

juliusmarminge
Copy link
Contributor

@juliusmarminge juliusmarminge commented Apr 5, 2024

Fixes #64018

Adds support for modern configurations introduced in TS 5.4

Expected output: not modifying the module, esModuleInterop configs since module: preserve is present

   We detected TypeScript in your project and reconfigured your tsconfig.json file for you. Strict-mode is set to false
 by default.
   The following suggested values were added to your tsconfig.json. These values can be changed to fit your project's n
eeds:

        - lib was set to dom,dom.iterable,esnext
        - allowJs was set to true
        - skipLibCheck was set to true
        - strict was set to false
        - noEmit was set to true
        - incremental was set to true
        - include was set to ['next-env.d.ts', '**/*.ts', '**/*.tsx']
        - exclude was set to ['node_modules']

   The following mandatory changes were made to your tsconfig.json:

        - isolatedModules was set to true (requirement for SWC / Babel)
        - jsx was set to preserve (next.js implements its own optimized jsx transform)

@ijjk
Copy link
Member

ijjk commented Apr 5, 2024

Allow CI Workflow Run

  • approve CI run for commit: d7694cd

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

@ijjk
Copy link
Member

ijjk commented Apr 5, 2024

Allow CI Workflow Run

  • approve CI run for commit: 474837d

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

@@ -1348,7 +1348,7 @@ export async function renderToHTMLImpl(
(initialStream: ReactReadableStream, suffix?: string) => {
return continueFizzStream(initialStream, {
suffix,
inlinedDataStream: serverComponentsInlinedTransformStream?.readable,
inlinedDataStream: (serverComponentsInlinedTransformStream as any)?.readable,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

side-effect from upgrading to ts 5.4. didn't look into the root cause

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can revert back to 5.2.2 and do some (ts.ModuleKind as any).Preserve in writeConfigurationDefaults, although I'm not sure if the tests will pass then since Preserve isn't recognized and might throw a build-error in the test

"module": "esnext",
"esModuleInterop": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this moved down a line to keep the code above a bit cleaner without needing multiple checks

Comment on lines +60 to +64
// TypeScript 5.4 introduced `Preserve`. Using `Preserve` implies
// - `moduleResolution` is `Bundler`
// - `esModuleInterop` is `true`
// - `resolveJsonModule` is `true`
// This means that if the user is using Preserve, they don't need these options
Copy link
Contributor Author

Choose a reason for hiding this comment

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

esModuleInterop: {
value: true,
reason: 'requirement for SWC / babel',
},
module: {
parsedValue: ts.ModuleKind.ESNext,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps Preserve should be the default if using 5.4 or above, but i didn't feel like updating all the tests. Can do so if you think it's a good idea

@huozhi
Copy link
Member

huozhi commented Apr 5, 2024

Thanks for the PR! We're trying to upgrade TS in another PR now, still working on the blockers. Could you revert the ts version bump change?
We could add a e2e test here to test if that works, which uses the installed ts version. We can also help to add the test for that there 🙏

@juliusmarminge
Copy link
Contributor Author

juliusmarminge commented Apr 5, 2024

Aight reverted and added an E2E test for now! @huozhi

@huozhi huozhi added the CI approved Approve running CI for fork label Apr 5, 2024
@ijjk
Copy link
Member

ijjk commented Apr 5, 2024

Failing test suites

Commit: aac9d2c

pnpm test test/integration/preload-viewport/test/index.test.js

  • Prefetching Links in viewport > production mode > should prefetch with non-bot UA
Expand output

● Prefetching Links in viewport › production mode › should prefetch with non-bot UA

expect(received).toHaveLength(expected)

Expected length: 1
Received length: 0
Received array:  []

  131 |           )
  132 |           const links = await browser.elementsByCss('link[rel=prefetch]')
> 133 |           expect(links).toHaveLength(1)
      |                         ^
  134 |         } finally {
  135 |           if (browser) await browser.close()
  136 |         }

  at Object.toHaveLength (integration/preload-viewport/test/index.test.js:133:25)

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

pnpm test-dev test/development/middleware-errors/index.test.ts

  • middleware - development errors > when there is a compilation error after boot > logs the error correctly
Expand output

● middleware - development errors › when there is a compilation error after boot › logs the error correctly

TIMED OUT: success

undefined

Error: expect(received).toEqual(expected) // deep equality

Expected: 2
Received: 3

  642 |
  643 |   if (hardError) {
> 644 |     throw new Error('TIMED OUT: ' + regex + '\n\n' + content + '\n\n' + lastErr)
      |           ^
  645 |   }
  646 |   return false
  647 | }

  at check (lib/next-test-utils.ts:644:11)
  at Object.<anonymous> (development/middleware-errors/index.test.ts:291:9)

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

@ijjk
Copy link
Member

ijjk commented Apr 5, 2024

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary juliusmarminge/next.js julius/tsconfig-module-preserve Change
buildDuration 14s 14s N/A
buildDurationCached 7.6s 6.2s N/A
nodeModulesSize 199 MB 199 MB ⚠️ +2.54 kB
nextStartRea..uration (ms) 406ms 398ms N/A
Client Bundles (main, webpack)
vercel/next.js canary juliusmarminge/next.js julius/tsconfig-module-preserve Change
2453-HASH.js gzip 31.4 kB 31.4 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.04 kB 5.04 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 32.2 kB 32.2 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 juliusmarminge/next.js julius/tsconfig-module-preserve Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary juliusmarminge/next.js julius/tsconfig-module-preserve 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.21 kB 4.21 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.57 kB 6.57 kB
Client Build Manifests
vercel/next.js canary juliusmarminge/next.js julius/tsconfig-module-preserve Change
_buildManifest.js gzip 481 B 484 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary juliusmarminge/next.js julius/tsconfig-module-preserve Change
index.html gzip 529 B 528 B N/A
link.html gzip 541 B 541 B
withRouter.html gzip 524 B 522 B N/A
Overall change 541 B 541 B
Edge SSR bundle Size
vercel/next.js canary juliusmarminge/next.js julius/tsconfig-module-preserve Change
edge-ssr.js gzip 95.4 kB 95.4 kB N/A
page.js gzip 3.06 kB 3.06 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary juliusmarminge/next.js julius/tsconfig-module-preserve Change
middleware-b..fest.js gzip 625 B 624 B N/A
middleware-r..fest.js gzip 151 B 151 B
middleware.js gzip 25.5 kB 25.5 kB N/A
edge-runtime..pack.js gzip 839 B 839 B
Overall change 990 B 990 B
Next Runtimes
vercel/next.js canary juliusmarminge/next.js julius/tsconfig-module-preserve Change
app-page-exp...dev.js gzip 170 kB 170 kB
app-page-exp..prod.js gzip 97 kB 97 kB
app-page-tur..prod.js gzip 98.8 kB 98.8 kB
app-page-tur..prod.js gzip 93 kB 93 kB
app-page.run...dev.js gzip 144 kB 144 kB
app-page.run..prod.js gzip 91.5 kB 91.5 kB
app-route-ex...dev.js gzip 21.4 kB 21.4 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.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 22.5 kB 22.5 kB
pages.runtim...dev.js gzip 23.1 kB 23.1 kB
pages.runtim..prod.js gzip 22.4 kB 22.4 kB
server.runti..prod.js gzip 51.1 kB 51.1 kB
Overall change 945 kB 945 kB
build cache Overall increase ⚠️
vercel/next.js canary juliusmarminge/next.js julius/tsconfig-module-preserve Change
0.pack gzip 1.58 MB 1.58 MB ⚠️ +643 B
index.pack gzip 106 kB 106 kB N/A
Overall change 1.58 MB 1.58 MB ⚠️ +643 B
Diff details
Diff for middleware.js

Diff too large to display

Commit: aac9d2c

@juliusmarminge
Copy link
Contributor Author

Can you help fix up the tests @huozhi? It's passing locally for me:

@huozhi huozhi enabled auto-merge (squash) April 8, 2024 11:17
@huozhi huozhi merged commit a7c24ac into vercel:canary Apr 8, 2024
74 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 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 locked type: next
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tsconfig generation doesn't recognize ts5.4 improvements
3 participants