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: support import attributes #59480

Merged
merged 11 commits into from
Apr 24, 2024

Conversation

chentsulin
Copy link
Contributor

@chentsulin chentsulin commented Dec 11, 2023

What?

Add support to import attributes.

Why?

The old import assertions syntax is deprecated and the proposal is updated. See babel/babel#15536

How?

Add support to import attributes and keep old import assertions working by using @babel/plugin-syntax-import-attributes with the deprecatedAssertSyntax option.

Docs: https://babeljs.io/docs/babel-plugin-syntax-import-attributes

@ijjk
Copy link
Member

ijjk commented Dec 11, 2023

Allow CI Workflow Run

  • approve CI run for commit: ff58490

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

Sorry, something went wrong.

2 similar comments
@ijjk
Copy link
Member

ijjk commented Dec 11, 2023

Allow CI Workflow Run

  • approve CI run for commit: ff58490

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

Sorry, something went wrong.

@ijjk
Copy link
Member

ijjk commented Dec 11, 2023

Allow CI Workflow Run

  • approve CI run for commit: ff58490

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

Sorry, something went wrong.

@huozhi huozhi added the CI approved Approve running CI for fork label Dec 11, 2023
Copy link
Member

@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.

Thanks for the PR, the integration test test/integration/import-assertion also needs to be updated. For swc options you also need to enable keepImportAttributes and drop keepImportAssertions flag

@chentsulin
Copy link
Contributor Author

Added an integration tests for the new with syntax.

@huozhi I can't find any keepImportAssertions in the current codebase and keepImportAttributes has already been enabled:

keepImportAttributes: true,

BTW, It seems that we need to upgrade prettier to 3.1.1 to support formatting import attributes in tests: https://github.com/prettier/prettier/releases/tag/3.1.1

@ijjk
Copy link
Member

ijjk commented Dec 11, 2023

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary chentsulin/next.js feat/support-import-attributes Change
buildDuration 18.2s 15.3s N/A
buildDurationCached 8s 7s N/A
nodeModulesSize 238 MB 238 MB ⚠️ +1.07 kB
nextStartRea..uration (ms) 382ms 380ms N/A
Client Bundles (main, webpack)
vercel/next.js canary chentsulin/next.js feat/support-import-attributes 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.09 kB 5.09 kB
framework-HASH.js gzip 45.2 kB 45.2 kB
main-app-HASH.js gzip 229 B 228 B N/A
main-HASH.js gzip 31.5 kB 31.6 kB N/A
webpack-HASH.js gzip 1.64 kB 1.65 kB N/A
Overall change 50.5 kB 50.5 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary chentsulin/next.js feat/support-import-attributes Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary chentsulin/next.js feat/support-import-attributes 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 510 B 510 B
css-HASH.js gzip 342 B 343 B N/A
dynamic-HASH.js gzip 2.51 kB 2.52 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.2 kB 1.2 kB
Client Build Manifests
vercel/next.js canary chentsulin/next.js feat/support-import-attributes Change
_buildManifest.js gzip 482 B 484 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary chentsulin/next.js feat/support-import-attributes Change
index.html gzip 529 B 528 B N/A
link.html gzip 541 B 541 B
withRouter.html gzip 524 B 523 B N/A
Overall change 541 B 541 B
Edge SSR bundle Size
vercel/next.js canary chentsulin/next.js feat/support-import-attributes Change
edge-ssr.js gzip 108 kB 108 kB N/A
page.js gzip 3.05 kB 3.04 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary chentsulin/next.js feat/support-import-attributes Change
middleware-b..fest.js gzip 624 B 625 B N/A
middleware-r..fest.js gzip 155 B 156 B N/A
middleware.js gzip 27.8 kB 27.8 kB N/A
edge-runtime..pack.js gzip 839 B 839 B
Overall change 839 B 839 B
Next Runtimes
vercel/next.js canary chentsulin/next.js feat/support-import-attributes 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 65.3 kB 65.3 kB
Overall change 960 kB 960 kB
build cache Overall increase ⚠️
vercel/next.js canary chentsulin/next.js feat/support-import-attributes Change
0.pack gzip 1.61 MB 1.61 MB N/A
index.pack gzip 106 kB 107 kB ⚠️ +269 B
Overall change 106 kB 107 kB ⚠️ +269 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__(7456);
-      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/react@18.2.0/node_modules/react/jsx-runtime.js
       var jsx_runtime = __webpack_require__(1527);
-      // EXTERNAL MODULE: ./node_modules/.pnpm/file+..+main-repo+packages+next+next-packed.tgz_react-dom@18.2.0_react@18.2.0/node_modules/next/image.js
-      var next_image = __webpack_require__(1577);
+      // EXTERNAL MODULE: ./node_modules/.pnpm/file+..+diff-repo+packages+next+next-packed.tgz_react-dom@18.2.0_react@18.2.0/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: b380076

@chentsulin chentsulin requested a review from huozhi December 11, 2023 11:13
@huozhi
Copy link
Member

huozhi commented Dec 11, 2023

@huozhi I can't find any keepImportAssertions in the current codebase and keepImportAttributes has already been enabled:
@chentsulin nice, looks like it's already updated, all good!

For prettier errors we could use prettier ignore comments, add a TODO comment and upgrade prettier later in separate PR

@chentsulin chentsulin requested a review from leerob as a code owner December 11, 2023 11:58
@chentsulin
Copy link
Contributor Author

chentsulin commented Dec 11, 2023

@huozhi Tried. The approach didn't work because there're parsing errors:
 So I added the files to the .prettierignore file instead.

✖ prettier found some errors. Please fix them and try committing again.


[error] test/integration/import-attributes/pages/es.js: SyntaxError: Unexpected token, expected "(" (3:33)
[error]   1 | // TODO: upgrade prettier to 3.1.1 to support import attributes
[error]   2 | // prettier-ignore
[error] > 3 | import data from '../data' with { type: 'json' }
[error]     |                                 ^
[error]   4 |
[error]   5 | export default function Es() {
[error]   6 |   return data.foo
[error] test/integration/import-attributes/pages/ts.ts: SyntaxError: ';' expected. (3:28)
[error]   1 | // TODO: upgrade prettier to 3.1.1 to support import attributes
[error]   2 | // prettier-ignore
[error] > 3 | import data from '../data' with { type: 'json' }
[error]     |                            ^
[error]   4 |
[error]   5 | export default function Ts() {
[error]   6 |   return data.foo
 ELIFECYCLE  Command failed with exit code 1.

@ijjk
Copy link
Member

ijjk commented Dec 11, 2023

Failing test suites

Commit: b380076

TURBOPACK=1 pnpm test-dev test/e2e/prerender.test.ts (turbopack)

  • Prerender > should log error in console and browser in development mode
Expand output

● Prerender › should log error in console and browser in development mode

page.reload: net::ERR_ABORTED; maybe frame was detached?
Call log:
  - waiting for navigation until "load"

  289 |   refresh(): BrowserInterface {
  290 |     return this.chain(async () => {
> 291 |       await page.reload()
      |                  ^
  292 |     })
  293 |   }
  294 |   setDimensions({

  at reload (lib/browsers/playwright.ts:291:18)

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

pnpm test-start test/production/deployment-id-handling/deployment-id-handling.test.ts

  • deployment-id-handling enabled with CUSTOM_DEPLOYMENT_ID > should append dpl query to all assets correctly for /
  • deployment-id-handling enabled with CUSTOM_DEPLOYMENT_ID > should append dpl query to all assets correctly for /pages-edge
  • deployment-id-handling enabled with CUSTOM_DEPLOYMENT_ID > should append dpl query to all assets correctly for /from-app
  • deployment-id-handling enabled with CUSTOM_DEPLOYMENT_ID > should append dpl query to all assets correctly for /from-app/edge
  • deployment-id-handling enabled with CUSTOM_DEPLOYMENT_ID > should have deployment id env available
  • deployment-id-handling enabled with CUSTOM_DEPLOYMENT_ID > should have deployment id env available
  • deployment-id-handling enabled with NEXT_DEPLOYMENT_ID > should append dpl query to all assets correctly for /
  • deployment-id-handling enabled with NEXT_DEPLOYMENT_ID > should append dpl query to all assets correctly for /pages-edge
  • deployment-id-handling enabled with NEXT_DEPLOYMENT_ID > should append dpl query to all assets correctly for /from-app
  • deployment-id-handling enabled with NEXT_DEPLOYMENT_ID > should append dpl query to all assets correctly for /from-app/edge
  • deployment-id-handling enabled with NEXT_DEPLOYMENT_ID > should have deployment id env available
  • deployment-id-handling enabled with NEXT_DEPLOYMENT_ID > should have deployment id env available
Expand output

● deployment-id-handling enabled with NEXT_DEPLOYMENT_ID › should append dpl query to all assets correctly for /

thrown: "Exceeded timeout of 120000 ms for a hook.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

  252 |   let next: NextInstance
  253 |   if (!skipped) {
> 254 |     beforeAll(async () => {
      |     ^
  255 |       next = await createNext(options)
  256 |     })
  257 |     afterAll(async () => {

  at beforeAll (lib/e2e-utils.ts:254:5)
  at production/deployment-id-handling/deployment-id-handling.test.ts:9:35
      at Array.forEach (<anonymous>)
  at Object.<anonymous> (production/deployment-id-handling/deployment-id-handling.test.ts:5:61)

● deployment-id-handling enabled with NEXT_DEPLOYMENT_ID › should append dpl query to all assets correctly for /pages-edge

thrown: "Exceeded timeout of 120000 ms for a hook.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

  252 |   let next: NextInstance
  253 |   if (!skipped) {
> 254 |     beforeAll(async () => {
      |     ^
  255 |       next = await createNext(options)
  256 |     })
  257 |     afterAll(async () => {

  at beforeAll (lib/e2e-utils.ts:254:5)
  at production/deployment-id-handling/deployment-id-handling.test.ts:9:35
      at Array.forEach (<anonymous>)
  at Object.<anonymous> (production/deployment-id-handling/deployment-id-handling.test.ts:5:61)

● deployment-id-handling enabled with NEXT_DEPLOYMENT_ID › should append dpl query to all assets correctly for /from-app

thrown: "Exceeded timeout of 120000 ms for a hook.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

  252 |   let next: NextInstance
  253 |   if (!skipped) {
> 254 |     beforeAll(async () => {
      |     ^
  255 |       next = await createNext(options)
  256 |     })
  257 |     afterAll(async () => {

  at beforeAll (lib/e2e-utils.ts:254:5)
  at production/deployment-id-handling/deployment-id-handling.test.ts:9:35
      at Array.forEach (<anonymous>)
  at Object.<anonymous> (production/deployment-id-handling/deployment-id-handling.test.ts:5:61)

● deployment-id-handling enabled with NEXT_DEPLOYMENT_ID › should append dpl query to all assets correctly for /from-app/edge

thrown: "Exceeded timeout of 120000 ms for a hook.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

  252 |   let next: NextInstance
  253 |   if (!skipped) {
> 254 |     beforeAll(async () => {
      |     ^
  255 |       next = await createNext(options)
  256 |     })
  257 |     afterAll(async () => {

  at beforeAll (lib/e2e-utils.ts:254:5)
  at production/deployment-id-handling/deployment-id-handling.test.ts:9:35
      at Array.forEach (<anonymous>)
  at Object.<anonymous> (production/deployment-id-handling/deployment-id-handling.test.ts:5:61)

● deployment-id-handling enabled with NEXT_DEPLOYMENT_ID › should have deployment id env available

thrown: "Exceeded timeout of 120000 ms for a hook.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

  252 |   let next: NextInstance
  253 |   if (!skipped) {
> 254 |     beforeAll(async () => {
      |     ^
  255 |       next = await createNext(options)
  256 |     })
  257 |     afterAll(async () => {

  at beforeAll (lib/e2e-utils.ts:254:5)
  at production/deployment-id-handling/deployment-id-handling.test.ts:9:35
      at Array.forEach (<anonymous>)
  at Object.<anonymous> (production/deployment-id-handling/deployment-id-handling.test.ts:5:61)

● deployment-id-handling enabled with NEXT_DEPLOYMENT_ID › should have deployment id env available

thrown: "Exceeded timeout of 120000 ms for a hook.
Add a timeout value to this test to increase the timeout, if this is a long-running test. See https://jestjs.io/docs/api#testname-fn-timeout."

  252 |   let next: NextInstance
  253 |   if (!skipped) {
> 254 |     beforeAll(async () => {
      |     ^
  255 |       next = await createNext(options)
  256 |     })
  257 |     afterAll(async () => {

  at beforeAll (lib/e2e-utils.ts:254:5)
  at production/deployment-id-handling/deployment-id-handling.test.ts:9:35
      at Array.forEach (<anonymous>)
  at Object.<anonymous> (production/deployment-id-handling/deployment-id-handling.test.ts:5:61)

● deployment-id-handling enabled with CUSTOM_DEPLOYMENT_ID › should append dpl query to all assets correctly for /

createNext called without destroying previous instance

  160 |   try {
  161 |     if (nextInstance) {
> 162 |       throw new Error(`createNext called without destroying previous instance`)
      |             ^
  163 |     }
  164 |
  165 |     setupTracing()

  at createNext (lib/e2e-utils.ts:162:13)
  at Object.createNext (lib/e2e-utils.ts:255:20)

● deployment-id-handling enabled with CUSTOM_DEPLOYMENT_ID › should append dpl query to all assets correctly for /pages-edge

createNext called without destroying previous instance

  160 |   try {
  161 |     if (nextInstance) {
> 162 |       throw new Error(`createNext called without destroying previous instance`)
      |             ^
  163 |     }
  164 |
  165 |     setupTracing()

  at createNext (lib/e2e-utils.ts:162:13)
  at Object.createNext (lib/e2e-utils.ts:255:20)

● deployment-id-handling enabled with CUSTOM_DEPLOYMENT_ID › should append dpl query to all assets correctly for /from-app

createNext called without destroying previous instance

  160 |   try {
  161 |     if (nextInstance) {
> 162 |       throw new Error(`createNext called without destroying previous instance`)
      |             ^
  163 |     }
  164 |
  165 |     setupTracing()

  at createNext (lib/e2e-utils.ts:162:13)
  at Object.createNext (lib/e2e-utils.ts:255:20)

● deployment-id-handling enabled with CUSTOM_DEPLOYMENT_ID › should append dpl query to all assets correctly for /from-app/edge

createNext called without destroying previous instance

  160 |   try {
  161 |     if (nextInstance) {
> 162 |       throw new Error(`createNext called without destroying previous instance`)
      |             ^
  163 |     }
  164 |
  165 |     setupTracing()

  at createNext (lib/e2e-utils.ts:162:13)
  at Object.createNext (lib/e2e-utils.ts:255:20)

● deployment-id-handling enabled with CUSTOM_DEPLOYMENT_ID › should have deployment id env available

createNext called without destroying previous instance

  160 |   try {
  161 |     if (nextInstance) {
> 162 |       throw new Error(`createNext called without destroying previous instance`)
      |             ^
  163 |     }
  164 |
  165 |     setupTracing()

  at createNext (lib/e2e-utils.ts:162:13)
  at Object.createNext (lib/e2e-utils.ts:255:20)

● deployment-id-handling enabled with CUSTOM_DEPLOYMENT_ID › should have deployment id env available

createNext called without destroying previous instance

  160 |   try {
  161 |     if (nextInstance) {
> 162 |       throw new Error(`createNext called without destroying previous instance`)
      |             ^
  163 |     }
  164 |
  165 |     setupTracing()

  at createNext (lib/e2e-utils.ts:162:13)
  at Object.createNext (lib/e2e-utils.ts:255:20)

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

pnpm test-dev test/e2e/app-dir/parallel-routes-revalidation/parallel-routes-revalidation.test.ts

  • parallel-routes-revalidation > should submit the action and revalidate the page data
  • parallel-routes-revalidation > should handle router.refresh() when called in a slot
  • parallel-routes-revalidation > should handle a redirect action when called in a slot
Expand output

● parallel-routes-revalidation › should submit the action and revalidate the page data

page.goto: Timeout 60000ms exceeded.
Call log:
  - navigating to "http://localhost:42303/", waiting until "load"

  274 |     opts?.beforePageLoad?.(page)
  275 |
> 276 |     await page.goto(url, { waitUntil: 'load' })
      |                ^
  277 |   }
  278 |
  279 |   back(options): BrowserInterface {

  at BrowserInterface.goto (lib/browsers/playwright.ts:276:16)
  at webdriver (lib/next-webdriver.ts:129:3)
  at Object.<anonymous> (e2e/app-dir/parallel-routes-revalidation/parallel-routes-revalidation.test.ts:11:23)

● parallel-routes-revalidation › should handle router.refresh() when called in a slot

page.goto: Timeout 60000ms exceeded.
Call log:
  - navigating to "http://localhost:42303/", waiting until "load"

  274 |     opts?.beforePageLoad?.(page)
  275 |
> 276 |     await page.goto(url, { waitUntil: 'load' })
      |                ^
  277 |   }
  278 |
  279 |   back(options): BrowserInterface {

  at BrowserInterface.goto (lib/browsers/playwright.ts:276:16)
  at webdriver (lib/next-webdriver.ts:129:3)
  at Object.<anonymous> (e2e/app-dir/parallel-routes-revalidation/parallel-routes-revalidation.test.ts:45:23)

● parallel-routes-revalidation › should handle a redirect action when called in a slot

page.goto: Timeout 60000ms exceeded.
Call log:
  - navigating to "http://localhost:42303/", waiting until "load"

  274 |     opts?.beforePageLoad?.(page)
  275 |
> 276 |     await page.goto(url, { waitUntil: 'load' })
      |                ^
  277 |   }
  278 |
  279 |   back(options): BrowserInterface {

  at BrowserInterface.goto (lib/browsers/playwright.ts:276:16)
  at webdriver (lib/next-webdriver.ts:129:3)
  at Object.<anonymous> (e2e/app-dir/parallel-routes-revalidation/parallel-routes-revalidation.test.ts:73:23)

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

@@ -0,0 +1,5 @@
import data from '../data' with { type: 'json' }
Copy link
Member

Choose a reason for hiding this comment

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

eslint is failing

/root/actions-runner/_work/next.js/next.js/test/integration/import-attributes/pages/ts.ts
  1:27  error  Parsing error: ';' expected

✖ 1 problem (1 error, 0 warnings)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a separate PR to upgrade typescript-eslint: #59514

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@huozhi Should I bump TS to 5.3.3 in this PR too? I'm not sure but it may fix it.

Copy link
Member

Choose a reason for hiding this comment

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

Let's bump TS in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do rebase after #59494 get merged.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it still can't parse the syntax correctly. I believe we need TypeScript 5.3 to solve the issue.

/root/actions-runner/_work/next.js/next.js/test/integration/import-attributes/pages/ts.ts
  1:27  error  Parsing error: ';' expected

✖ 1 problem (1 error, 0 warnings)

Copy link
Member

Choose a reason for hiding this comment

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

@chentsulin We've upgraded typescript to 5.3 on canary, you can update the branch now 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The branch has been updated. 🙏

Copy link
Member

Choose a reason for hiding this comment

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

Looks like there are still merge conflicts in pnpm-lock.yaml

Maybe you can try regenerating it from the main branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @styfle, I can't find any merge conflicts in pnpm-lock.yaml.
After I merged the canary branch and run pnpm install again, nothing was changed in the pnpm-lock.yaml file.
Is there anything I am doing wrong?

@ijjk ijjk added the tests label Apr 23, 2024
@chentsulin chentsulin requested review from styfle and huozhi April 23, 2024 06:14
@styfle styfle enabled auto-merge (squash) April 24, 2024 14:10
@styfle styfle merged commit 5caeea3 into vercel:canary Apr 24, 2024
75 checks passed
kwonoj added a commit that referenced this pull request Apr 24, 2024
@kwonoj
Copy link
Contributor

kwonoj commented Apr 24, 2024

@chentsulin appreciate for the effort, but for now we reverted this change in #65001.

While debugging test failures, found out newly added test's assertion is too wide to misdetermine failing tests in turbopack as passing (#65000), which means this change does not work correctly on the turbopack.

kwonoj added a commit that referenced this pull request Apr 24, 2024
Reverts #59480

This PR have a test with incorrect assertion supposed to fail on CI with
turbopack.

#65000
@github-actions github-actions bot added the locked label May 9, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants