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

Fixture do not work when esbuild target is set to ES2015, ES2016 or esbuild.supported.['async-await'] = false #6042

Closed
6 tasks done
kasperpeulen opened this issue Jul 6, 2024 · 5 comments · Fixed by #6531 or #6575

Comments

@kasperpeulen
Copy link

kasperpeulen commented Jul 6, 2024

Describe the bug

I'm not 100% sure if this is a bug or by design.

If you set the vite config to target ES2015, async/await will be transpiled and Vitest can not analyse the fn.toString() call correctly to determine where the fixture depends on.

import { defineConfig } from 'vite';

export default defineConfig({
  esbuild: {
    target: 'ES2015',
    // OR
    target: 'ES2016',
    // OR
    supported: {
      'async-await': false,
    }
  },
});

With the following test:

import { test as base, expect } from 'vitest';

// Edit an assertion and save to see HMR in action

const test = base.extend({
  todos: async ({}, use) => {
    await use(['todo']);
  },
});

test('fixture', ({ todos }) => {
  expect(todos).toContain('todo');
});

Gives the following error:

 ❯ test/basic.test.ts (0)

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Suites 1 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯

 FAIL  test/basic.test.ts [ test/basic.test.ts ]
Error: The first argument inside a fixture must use object destructuring pattern, e.g. ({ test } => {}). Instead, received "_0".
 ❯ eval test/basic.test.ts:5:19
      3| // Edit an assertion and save to see HMR in action
      4| 
      5| const test = base.extend({
       |                   ^
      6|   todos: async ({}, use) => {
      7|     await use(['todo']);

⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯

 Test Files  1 failed (1)
      Tests  no tests
   Start at  20:36:33
   Duration  1.02s (transform 36ms, setup 0ms, collect 0ms, tests 0ms, environment 1ms, prepare 233ms)


 FAIL  Tests failed. Watching for file changes...
       press h to show help, press q to quit

See the reproduction.

In storybook we are considering also adopting fixture, and we started to use some similar as vitest to implement the mount RFC:
storybookjs/storybook#27389

Which you can see as the first fixture.

However, we found that our fixture code breaks in Angular, which always transpiles away async/await when Zone.js is used.

When looking at the output code of esbuild, the destructure gets kind of preserved:

const Story = {
    async play ({mount}) {
        console.log(mount)
    },
}

gets transpiled to:

const Story = {
  play(_0) {
    return __async(this, arguments, function* ({ mount }) {
      console.log(mount);
    });
  }
};

So, it might be possible to improve the regex to cover to extract the fixture dependency information reliably.

My general feeling is that it might be just to fragile, but I am wondering what you here think about this.

Reproduction

https://stackblitz.com/edit/vitest-dev-vitest-tdu2jo?file=test%2Fbasic.test.ts

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (8) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.20.3 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.2.3 - /usr/local/bin/npm
    pnpm: 8.15.6 - /usr/local/bin/pnpm
  npmPackages:
    @vitest/ui: latest => 1.6.0 
    vite: latest => 5.3.3 
    vitest: latest => 1.6.0

Used Package Manager

npm

Validations

@sheremet-va
Copy link
Member

sheremet-va commented Jul 6, 2024

Interesting point, but I wonder what is the point of setting esbuild to es5 in Vitest since we don't even support any runtimes with that target (the lowest we support is Node18 which is way ahead of es5). Maybe in our case we can forcefully set it higher 🤔

@kasperpeulen
Copy link
Author

kasperpeulen commented Jul 6, 2024

@sheremet-va
Note that this is about using esbuild target ES2015, not ES5.

But a valid case would be that the user uses Angular with vitest.

Angular can not use async/await natively if zone.js is used
https://angular.dev/guide/experimental/zoneless#why-use-zoneless

ZoneJS works by patching browser APIs but does not automatically have patches for every new browser API. Some APIs simply cannot be patched effectively, such as async/await, and have to be downleveled to work with ZoneJS.

In this case the user must set esbuild to ES2015, ES2016 (or esbuild.supported.['async-await'] = false), but for any of those settings, fixture don't work anymore as reproduced above, as the following regex won't match:

function getUsedProps(fn: Function) {
const match = fn.toString().match(/[^(]*\(([^)]*)/)
if (!match) {
return []
}

Edit the following config also won't work with fixtures:

    target: 'ES2015',
    // OR
    target: 'ES2016',
    // OR
    supported: {
      'async-await': false,
    }

That being said, angular is moving away from Zone.js for those kind of reasons.
And you can already disable zoneless.js:
https://angular.dev/guide/experimental/zoneless
image

@kasperpeulen kasperpeulen changed the title Fixture do not work when esbuild target is set to ES2015 Fixture do not work when esbuild target is set to ES2015, ES2016 or esbuild.supported.['async-await'] = false Jul 7, 2024
@sheremet-va
Copy link
Member

Then I think we can just support esbuild's __async output in the regexp since we know esbuild is the compiler.

@kasperpeulen
Copy link
Author

@hi-ogawa Thanks for working on this!

I tried it out, but there are still cases that break. For example, async method() syntax gives an error:

const test = base.extend<Fixture>({
  simple: async ({}, use) => {
    await use("simple");
  },
  async method ({ simple }, use) {
    await use("method:" + simple);
  },
});
Error: The first argument inside a fixture must use object destructuring pattern, e.g. ({ test } => {}). Instead, received "_0".
    at getUsedProps (file:///Users/kasperpeulen/code/github/vitest/packages/runner/dist/index.js:247:11)
    at file:///Users/kasperpeulen/code/github/vitest/packages/runner/dist/index.js:119:25
    at Array.forEach (<anonymous>)
    at mergeContextFixtures (file:///Users/kasperpeulen/code/github/vitest/packages/runner/dist/index.js:117:16)
    at Function.taskFn.extend (file:///Users/kasperpeulen/code/github/vitest/packages/runner/dist/index.js:667:22)
    at /Users/kasperpeulen/code/github/vitest/test/config/fixtures/fixture-no-async/basic.test.ts:9:19
    at VitestExecutor.runModule (file:///Users/kasperpeulen/code/github/vitest/packages/vite-node/dist/client.mjs:399:5)
    at VitestExecutor.directRequest (file:///Users/kasperpeulen/code/github/vitest/packages/vite-node/dist/client.mjs:381:5)
    at VitestExecutor.cachedRequest (file:///Users/kasperpeulen/code/github/vitest/packages/vite-node/dist/client.mjs:206:14)
    at VitestExecutor.executeId (file:///Users/kasperpeulen/code/github/vitest/packages/vite-node/dist/client.mjs:173:12)

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Sep 25, 2024

@kasperpeulen Hmm, interesting. I thought we only supported arrow functions for fixtures, so haven't considered that case.

I've only seen arrow functions, but the doc has this too https://vitest.dev/guide/test-context.html#fixture-initialization, which probably also breaks. Let me check it again.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants