Skip to content

refactor(turbopack): Refactor turbo-tasks-fs to take &self instead #80633

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: canary
Choose a base branch
from
Open

refactor(turbopack): Refactor turbo-tasks-fs to take &self instead #80633

wants to merge 2 commits into from
+20 −24

Conversation

kdy1
Copy link
Member

@kdy1 kdy1 commented Jun 18, 2025 •
edited
Loading

What?

Take &self from some trivial methods.

Why?

To prepare Vc<FileSystemPath> to FileSystemPath.

@ijjk ijjk added created-by: Turbopack team PRs by the Turbopack team. Turbopack Related to Turbopack with Next.js. labels Jun 18, 2025
@kdy1 Graphite App
Copy link
Member Author

kdy1 commented Jun 18, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@kdy1 kdy1 changed the title Use &self refactor(turbopack): Refactor turbo-tasks-fs to take &self instead Jun 18, 2025
@kdy1 kdy1 requested a review from a team June 18, 2025 00:04
@kdy1 kdy1 marked this pull request as ready for review June 18, 2025 00:04
@kdy1 kdy1 mentioned this pull request Jun 18, 2025
Draft
@ijjk
Copy link
Member

ijjk commented Jun 18, 2025 •
edited
Loading

Failing test suites

Commit: 7c2c1ff

pnpm test-dev test/development/app-dir/missing-required-html-tags/index.test.ts

  • app-dir - missing required html tags > should hmr when you fix the error
Expand output

● app-dir - missing required html tags › should hmr when you fix the error

expect(received).toMatchInlineSnapshot(snapshot)

Snapshot name: `app-dir - missing required html tags should hmr when you fix the error 1`

- Snapshot  - 1
+ Received  + 1

@@ -1,7 +1,7 @@
  {
-   "description": "Missing <html> tags in the root layout.
+   "description": "Missing <html> and <body> tags in the root layout.
  Read more at https://nextjs.org/docs/messages/missing-root-layout-tags",
    "environmentLabel": null,
    "label": "Runtime Error",
    "source": null,
    "stack": [],

  45 |     await assertHasRedbox(browser)
  46 |
> 47 |     await expect(browser).toDisplayRedbox(`
     |                           ^
  48 |      {
  49 |        "description": "Missing <html> tags in the root layout.
  50 |      Read more at https://nextjs.org/docs/messages/missing-root-layout-tags",

  at Object.toDisplayRedbox (development/app-dir/missing-required-html-tags/index.test.ts:47:27)

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

pnpm test-dev-turbo test/e2e/app-dir/actions/app-action.test.ts (turbopack)

  • app-dir action handling > should forward action request to a worker that contains the action handler (node)
  • app-dir action handling > should forward action request to a worker that contains the action handler (edge)
Expand output

● app-dir action handling › should forward action request to a worker that contains the action handler (node)

page.waitForSelector: Timeout 10000ms exceeded.
Call log:
  - waiting for locator('#other-page')

  454 |   waitForElementByCss(selector: string, timeout = 10_000) {
  455 |     return this.startChain(async () => {
> 456 |       const el = await page.waitForSelector(selector, {
      |                             ^
  457 |         timeout,
  458 |         state: 'attached',
  459 |       })

  at waitForSelector (lib/browsers/playwright.ts:456:29)
  at Playwright._chain (lib/browsers/playwright.ts:568:23)
  at Playwright._chain [as startChain] (lib/browsers/playwright.ts:549:17)
  at Playwright.startChain [as waitForElementByCss] (lib/browsers/playwright.ts:455:17)
  at waitForElementByCss (e2e/app-dir/actions/app-action.test.ts:908:21)

● app-dir action handling › should forward action request to a worker that contains the action handler (edge)

page.waitForSelector: Timeout 10000ms exceeded.
Call log:
  - waiting for locator('#other-page')

  454 |   waitForElementByCss(selector: string, timeout = 10_000) {
  455 |     return this.startChain(async () => {
> 456 |       const el = await page.waitForSelector(selector, {
      |                             ^
  457 |         timeout,
  458 |         state: 'attached',
  459 |       })

  at waitForSelector (lib/browsers/playwright.ts:456:29)
  at Playwright._chain (lib/browsers/playwright.ts:568:23)
  at Playwright._chain [as startChain] (lib/browsers/playwright.ts:549:17)
  at Playwright.startChain [as waitForElementByCss] (lib/browsers/playwright.ts:455:17)
  at waitForElementByCss (e2e/app-dir/actions/app-action.test.ts:908:21)

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

pnpm test-dev test/e2e/app-dir/fallback-shells/fallback-shells.test.ts

  • fallback-shells > without IO > should start and not postpone the response
Expand output

● fallback-shells › without IO › should start and not postpone the response

thrown: "Timed out waiting for the response of /without-io/world"

   8 |
   9 |   describe('without IO', () => {
> 10 |     it('should start and not postpone the response', async () => {
     |     ^
  11 |       const { browser, response } =
  12 |         await next.browserWithResponse('/without-io/world')
  13 |

  at it (e2e/app-dir/fallback-shells/fallback-shells.test.ts:10:5)
  at describe (e2e/app-dir/fallback-shells/fallback-shells.test.ts:9:3)
  at Object.describe (e2e/app-dir/fallback-shells/fallback-shells.test.ts:4:1)

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

@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Jun 18, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #80633 will not alter performance

Comparing kdy1/filepath-1 (db5bcdb) with canary (e1b9236)

Summary

✅ 12 untouched benchmarks

lukesandberg
lukesandberg reviewed Jun 18, 2025
View reviewed changes
turbopack/crates/turbo-tasks-fs/src/lib.rs
let this = self.await?;
if let Some(path) = join_path(&this.path, &path) {
Ok(Self::new_normalized(*this.fs, path.into()))
pub fn join(&self, path: RcStr) -> Result<Vc<Self>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

dropping async also tags this as immutable

is that correct? since it needs to read `Vc`` to be executed?

This is kind of unrelated to your PR... sorry

Copy link
Member Author

@kdy1 kdy1 Jun 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That was intentional, but I have thought about it again because of your comment. Thank you.
I think we may need to distinguish &self and self: Vc<Self> from the immutability detector, but not sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

My guess was that, if it's a problem, CI would catch it

Copy link
Contributor

@mischnic mischnic Jun 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

These two are equivalent, so you probably need some handling for this?

  • taking &self
  • self: Vc<Self> and then doing self.await

Copy link
Member Author

Choose a reason for hiding this comment

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

I made #80646

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think we have a ton of test coverage for invalidations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukesandberg lukesandberg lukesandberg left review comments

@mischnic mischnic mischnic left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
created-by: Turbopack team PRs by the Turbopack team. Turbopack Related to Turbopack with Next.js.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kdy1 @ijjk @lukesandberg @mischnic