fix: Do not hoist imports whose names match class properties (fix #10093).

[SunsetFi]

fix: Do not hoist imports whose names match class properties (fix #10093).

Description

Fixes vitest hoisting imports when a class property shares the same name.

Resolves #10093

Full reproduction

When vitest 4.1.3 encounters a class property that matches the name of an import, it attempts to hoist the import above the class property, often resulting in reference errors:

const { myMethodMock } = vi.hoisted(() => ({
  myMethodMock: vi.fn(),
}));

vi.mock("./MyClass", () => ({
  MyClass: class {
    // This line triggers the crash
    myMethod = myMethodMock;
    // This workaround fixes it:
    // ["myMethod"] = myMethodMock;
  },
}));

// This incorrectly gets hoisted above the MyClass mock,
// presumably because vitest sees the property name and thinks its accessing
// this function.
import { myMethod } from "./my-method";

describe("myMethod", () => {
  it("should call MyClass.myMethod", () => {
    myMethodMock.mockReturnValue("Mocked Hello, World!");
    const result = myMethod();
    expect(myMethodMock).toHaveBeenCalled();
    expect(result).toBe("Mocked Hello, World!");
  });
});

Since the property name is not a reference to the import, the hoisting is unnecessary, and hoisting itself causes a ReferenceError as it accesses the vite import of the module before the actual import is performed.

I am familiar with javascript parsing, but not too familiar with estree or vitest's internals. This PR is mostly a hunch, and I am not sure of the full ramifications of the change. However, it fixes the issue in my bug example repo.

There is a test at test/core/test/injector-mock.test.ts that seems to explicitly test for the faulty behavior. That is, it looks for the add and remove variables being hoisted above the class even though the class does not reference them. Fixing this bug caused this test to fail, as now there is no need to hoist the variables at all. I tweaked the test to reference the imports, and checked that the imports are still referenced correctly. However, I am not sure why this test existed in its original form, and I don't know if I'm missing some reason as to why this original behavior is actually the desired one.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
• Ideally, include a test that fails without this PR but passes with it.
• Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.
• Please check Allow edits by maintainers to make review process faster. Note that this option is not available for repositories that are owned by Github organizations.

Tests

• Run the tests with pnpm test:ci.
• Introduce a new test to check for this behavior

Changesets

• Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

[sheremet-va]

There is a test at test/core/test/injector-mock.test.ts that seems to explicitly test for the faulty behavior. That is, it looks for the add and remove variables being hoisted above the class even though the class does not reference them. Fixing this bug caused this test to fail, as now there is no need to hoist the variables at all. I tweaked the test to reference the imports, and checked that the imports are still referenced correctly. However, I am not sure why this test existed in its original form, and I don't know if I'm missing some reason as to why this original behavior is actually the desired one.

I think it is just being cautious. In your PR this is processed incorrectly:

hoistSimpleCodeWithoutMocks(
        `
import { remove, add, update } from 'vue'

class A {
  remove = 1
  add = null
  update = update // notice update is not transformed
}

remove(2);
add(4);
`,
      )

[SunsetFi]

Thank you, I pushed a fix for that case and added it to the test.

[SunsetFi]

I expanded the test with more access types:

import { remove, add, update, del, call } from 'vue'

class A {
  // No hoisting - good
  remove = 1
  // No hoisting - good
  add = null
  // Hoisting - good
  update = update
  // No hoisting, fine as the function captures the scope
  // del = () => __vi_import_0__.del()
  del = () => del()
  // No hoisting - Is this correct?
  // call = __vi_import_0__.call(4)
  call = call(4)
}

Most cases look good, but I'm not sure if we want to hoist the invocation of call or not. I would have expected the tree walk to enter the property value node and handle this appropriately, but I'm not sure if this behavior is what it 'should' be.

Debugging the test, the CallExpression => 'call' does return true for a reference, and is added to the identifiers array, but the hoisting never occurs...

[SunsetFi]

Well, that seems to have made the CI pass.

I'm trying to wrap my head around why the hoisting is needed at all. I think the reason for it is to ensure the name is preserved for NamedEvaluation in the spec; so that the name of the assignment inherited from the variable name is the same as the original code. I'm not confident on that determination though.

But if that's true, I believe a NamedEvaluation isn't relevant for the property when its set to the result of a call expression, so I don't think hoisting call here is required.

If i'm wrong, though, the bug lies elsewhere in vitest, as esmWalker still calls onIdentifier for it, albeit with false for all 3 options.

[sheremet-va]

It seems like the current behavior was added to fix this issue - #303

[SunsetFi]

Looking at the issue, I believe the fix introduced by #303 is preserved; the property 'add', which shares a name with an import, is not transformed as was happening prior to that fix. The test still covers this case.

Interestingly, the hoisting seems almost incidental to the bug fix, which was that class property keys were being transformed as imports. Unfortunately, that still leaves me with no clear idea why the hoisting itself was required.

[sheremet-va]

Interestingly, the hoisting seems almost incidental to the bug fix, which was that class property keys were being transformed as imports. Unfortunately, that still leaves me with no clear idea why the hoisting itself was required.

I think hoisting is a side effect of the way the transform works - vitejs/vite#6261 I'm not sure it was intended to be hoisted. The if check was initially applied only to class declarations

[SunsetFi]

Ya, I think I agree this might be unintended. All the code surrounding the classDeclaration usage does seem to only want class declarations, even beyond the name of the variable.

Reverting its computation to:

const classDeclaration = (parent.type === 'ClassDeclaration' && node === parent.superClass)

doesn't seem to have broken anything, and the bug in #303 still remains fixed.

The condition itself makes much more sense for hoisting, as we want to pull up a superclass that the parent class is dependent on.

The test:ci tests seem to mostly pass locally, but I am having difficulty getting playwright to run on my arch install. I'll push it up experimentally to see if the GHA runner passes it.

[SunsetFi]

There seems to be a test for the hoisting behavior here:

  // #2221
  test('should declare variable for imported super class', () => {
    expect(
      hoistSimpleCodeWithoutMocks(
        `import { Foo } from './dependency';` + `class A extends Foo {}`,
      ),
    ).toMatchInlineSnapshot(`
      "vi.mock('faker');
      const __vi_import_0__ = await import("./dependency");
      import {vi} from "vitest";
      const Foo = __vi_import_0__.Foo;
      class A extends Foo {}"
    `)

    // exported classes: should prepend the declaration at root level, before the
    // first class that uses the binding
    expect(
      hoistSimpleCodeWithoutMocks(
        `import { Foo } from './dependency';`
        + `export default class A extends Foo {}\n`
        + `export class B extends Foo {}`,
      ),
    ).toMatchInlineSnapshot(`
      "vi.mock('faker');
      const __vi_import_0__ = await import("./dependency");
      import {vi} from "vitest";
      const Foo = __vi_import_0__.Foo;
      export default class A extends Foo {}
      export class B extends Foo {}"
    `)
  })

It's marked #2221, but I can't find that issue or PR in the vitest repository.

[sheremet-va]

It's marked #2221, but I can't find that issue or PR in the vitest repository.

This transform is based on Vite's implementation so most of the # reference vite's repo: vitejs/vite#2221

[SunsetFi]

Okay, so the purpose isn't actually hoisting, but just getting the appropriate reference. That makes a lot more sense.

I definitely think aliasing the properties was an unintended bug by #303, and we can probably remove it.

So in this PR, esmWalker now correctly outputs identifiers used in the assignment portion of class properties, but not for the properties themselves. It also now no longer indicates that such references are class declarations, resulting in the references no longer getting aliased.

I also think I was off base with my assumption that it was a named reference thing. Thinking about it more, I believe the name for the named reference is always set by the assignment target, not the reference identifier. So I don't think removing the aliasing is going to result in observable differences to tests, although I might be wrong there.

[sheremet-va]

Can you also port the fix to Vite repo? It's here: https://github.com/vitejs/vite/blob/05803be47e88a0a0f2b792f0d9b819770bd23dbf/packages/vite/src/node/ssr/ssrTransform.ts#L367

[SunsetFi]

Can you also port the fix to Vite repo? It's here: https://github.com/vitejs/vite/blob/05803be47e88a0a0f2b792f0d9b819770bd23dbf/packages/vite/src/node/ssr/ssrTransform.ts#L367

Certainly, I will get to this in the next day.

---

I'm not sure what's causing tests to fail, only on macos, and only for a splattering of completely unrelated tests.

For the rolldown tests, the vite-ui tests are failing. Its looking for a button, found it, but then waits too long to click it and it disappears?

Error: locator.click: Test timeout of 30000ms exceeded.
      Call log:
        - waiting for getByRole('button', { name: 'Run current file' })
          - locator resolved to <button rounded="" opacity="70" role="button" text-green-700="" data-v-7f38aded="" dark:text-green-500="" hover="bg-active op100" data-testid="btn-run-test" aria-label="Run current file" class="w-1.4em h-1.4em flex v-popper--has-tooltip">…</button>
        - attempting click action
          - waiting for element to be visible, enabled and stable
          - element is not stable
        - retrying click action
          - waiting for element to be visible, enabled and stable
          - element is not visible
        - retrying click action
          - waiting 20ms
          2 × waiting for element to be visible, enabled and stable
            - element is not visible
          - retrying click action
            - waiting 100ms
          50 × waiting for element to be visible, enabled and stable
             - element is not visible
           - retrying click action
             - waiting 500ms

And for Build&Test, it seems to be missing some stdout content:

test/pool.test.ts > threads > can capture worker's stdout and stderr 191ms
     → expected 'Worker writing to stderr' to contain 'MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 3 message listeners added to [TestFixturesCustomEmitter]'

Neither error reproduces locally for me, and neither is happening for the other OSes in the pipeline.

These tests seemed to have failed earlier, but all of them passed in at least one commit. I want to blame test flakiness, especially because b08d4b8 and b937428 should be identical content.