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

Error in Browser mode x Node APIs #6293

Open
6 tasks done
Jinjiang opened this issue Aug 7, 2024 · 15 comments
Open
6 tasks done

Error in Browser mode x Node APIs #6293

Jinjiang opened this issue Aug 7, 2024 · 15 comments
Labels
p2-to-be-discussed Enhancement under consideration (priority)

Comments

@Jinjiang
Copy link

Jinjiang commented Aug 7, 2024

Describe the bug

It got error when run Vitest via Node APIs with config like this:

const viteConfig = {
  configFile: false,
  envFile: false,
  plugins: [vue()],
  test: {
    root,
    watch: false,
    browser: {
      enabled: true,
      provider: 'playwright',
      name: 'chromium',
      headless: true,
    }
  }
};

startVitest('test', undefined, undefined, viteConfig);

Error log:

11:58:25 AM [vite] Pre-transform error: Failed to parse source for import analysis because the content contains invalid JS syntax. Install @vitejs/plugin-vue to handle .vue files.
11:58:25 AM [vite] Internal server error: Failed to parse source for import analysis because the content contains invalid JS syntax. Install @vitejs/plugin-vue to handle .vue files.
  Plugin: vite:import-analysis
  File: /xxx/reproductions/App.vue:2:18
  1  |  <template>
  2  |    <h1>My App</h1>
     |                   ^
  3  |  </template>
      at TransformPluginContext._formatError (file:///xxx/reproductions/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/chunks/dep-mCdpKltl.js:48945:41)
      at TransformPluginContext.error (file:///xxx/reproductions/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/chunks/dep-mCdpKltl.js:48940:16)
      at TransformPluginContext.transform (file:///xxx/reproductions/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/chunks/dep-mCdpKltl.js:63667:14)
      at async PluginContainer.transform (file:///xxx/reproductions/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/chunks/dep-mCdpKltl.js:48786:18)
      at async loadAndTransform (file:///xxx/reproductions/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/chunks/dep-mCdpKltl.js:51608:27)
      at async viteTransformMiddleware (file:///xxx/reproductions/node_modules/.pnpm/[email protected]_@[email protected]/node_modules/vite/dist/node/chunks/dep-mCdpKltl.js:61557:24)
 ❯ App.vue.spec.ts (0)

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

 FAIL  App.vue.spec.ts [ App.vue.spec.ts ]
TypeError: Failed to fetch dynamically imported module: http://localhost:5173/xxx/reproductions/App.vue.spec.ts?import&browserv=1723003105187
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/1]⎯

 Test Files  1 failed (1)
      Tests  no tests
   Start at  11:58:24
   Duration  1.31s (transform 0ms, setup 0ms, collect 0ms, tests 0ms, environment 0ms, prepare 268ms)

 ELIFECYCLE  Command failed with exit code 1.

Reproduction

https://github.com/Jinjiang/reproductions/tree/try-vitest-browser-mode-20240807

pnpm install

# it works
pnpm test
# it doesn't work
pnpm test:debug

System Info

System:
    OS: macOS 15.0
    CPU: (8) arm64 Apple M1
    Memory: 53.38 MB / 8.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.1 - ~/.local/share/mise/installs/node/20/bin/node
    Yarn: 1.22.18 - ~/.yarn/bin/yarn
    npm: 10.2.4 - ~/.local/share/mise/installs/node/20/bin/npm
    pnpm: 9.6.0 - ~/Library/pnpm/pnpm
    bun: 1.0.29 - ~/.local/share/mise/installs/bun/latest/bin/bun
  Browsers:
    Safari: 18.0
  npmPackages:
    @vitejs/plugin-vue: ^5.1.2 => 5.1.2 
    @vitest/browser: ^2.0.5 => 2.0.5 
    vite: ^5.3.5 => 5.3.5 
    vitest: ^2.0.5 => 2.0.5

Used Package Manager

pnpm

Validations

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Aug 7, 2024

It looks like a duplicate of #5015
Vitest uses a separate Vite server for browser mode (or any workspace project) while viteConfig passed to startVitest only affects a main vite server.

Currently it doesn't look possible to setup workspace project level config from Node API like startVitest, so you might need an actual config file to pass vue() to browser mode project.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Aug 7, 2024

Looking at the reproduction, I just noticed you have vite config already, so I'm wondering if there's any reason you're disabling configFile? I think you can comment out these and it would work more naturally as it loads project vite.config.ts:

const viteConfig = {
  // configFile: false,
  // envFile: false,
  // plugins: [vue()],
  test: {
    // root: resolve(__dirname, '../'),
    watch: false,
    browser: {
      enabled: true,
      provider: 'playwright',
      name: 'chromium',
      headless: true,
    }
  }
};

@Jinjiang
Copy link
Author

Jinjiang commented Aug 7, 2024

@hi-ogawa thanks for the informaiton.

The background is I'm integrating vitest into @teambit in which there is no vite config file to resolve. The vite.config.ts in this reproduction is just for comparison between CLI command and Node API calls.

It would be great if we can support it for browser server in any way.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Aug 7, 2024

The background is I'm integrating vitest into @teambit in which there is no vite config file to resolve.

Can you elaborate more on this? For example, is it the assumption that users might not be even using Vite for their app/lib but want to use Vitest for testing?

@Jinjiang
Copy link
Author

Jinjiang commented Aug 7, 2024

Sure.

In short, Bit is a component-based workspace. In Bit, every component is based on a certain dev env. And every dev env (such as React env, Vue env, Angular env, etc) consists of multiple separate dev services like compiler, linter, formatter, tester, docs preview. By design, the dev env author can pick up any tools from the open source community and integrate them into the env.

e.g. for this vue env: https://bit.cloud/bitdev/vue/envs/vue-modern-env it picks up Vite as the docs preview and Vitest as the tester. You can also replace Vite into webpack as the docs preview but still use Vitest as the tester if you want. So the answer to your question is yes.

All the dev services are called via Node APIs in Bit. And the file structure of a workspace is slightly different from a normal workspace. So for flexibility, we usually prefer Node API calls and inline configs rather than CLI command and config files. That also means there is no vite.config.ts in each component dir. They are all configured in the corresponding dev env via inline config.

If you are interested, this is the source code that we currently resolve config for Vitest dev service:
https://bit.cloud/teambit/vite/vitest-tester/~code/utils.ts#l103
and we make the call here to run Vitest with the inline config:
https://bit.cloud/teambit/vite/vitest-tester/~code/vitest-tester.ts#l98

Let me know if you need any other information.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Aug 7, 2024

e.g. for this vue env: https://bit.cloud/bitdev/vue/envs/vue-modern-env it picks up Vite as the docs preview and Vitest as the tester. You can also replace Vite into webpack as the docs preview but still use Vitest as the tester if you want. So the answer to your question is yes.

Thanks, this context is useful to know 👍

For this use case, you'll need a way to pass plugins to browser project server via startVitest but currently that's not possible as in #5015.

Not sure, but maybe we can support something like inline test.workspace config, which goes like:

// run.js
import { startVitest, defineWorkspace } from 'vitest/node';
import vue from '@vitejs/plugin-vue';

await startVitest('test', undefined, undefined, {
  test: {
    // not supported
    workspace: defineWorkspace([
      {
        plugins: [vue()],
        test: {
          browser: {
            enabled: true,
            provider: 'playwright',
            name: 'chromium',
          }
        }
      }
    ])
  }
});

Currently test.workspace only supports a file path https://vitest.dev/config/#workspace, so the configuration needs to be split like:

//// run.js
import { startVitest } from 'vitest/node';
import vue from '@vitejs/plugin-vue';

await startVitest('test', undefined, undefined, {
  test: {
    workspace: "workspace.js"
  }
});

//// workspace.js
import { defineWorkspace } from 'vitest/node';
import vue from '@vitejs/plugin-vue';

export default defineWorkspace([
  {
    plugins: [vue()],
    test: {
      browser: {
        enabled: true,
        provider: 'playwright',
        name: 'chromium',
      }
    }
  }
])

Or instead of going through workspace, we could simply allow sneaking in plugins from test.browser.plugins config. 🤔

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Aug 7, 2024

@Jinjiang One question. What is this vitest config appearing under vue-modern-env? https://bit.cloud/bitdev/vue/envs/vue-modern-env/~code/config/vitest.config.mjs Isn't this something vitest-tester can pick up automatically as vitest config?

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Aug 7, 2024

I think the answer to that might be yes, but probably it gets merged as inline config instead of used as configFile option https://bit.cloud/teambit/vite/vitest-tester/~code/utils.ts#l160
I wonder what's the reason to keep configFile: false instead of passing configFile: configPath directly there.

@Jinjiang
Copy link
Author

Jinjiang commented Aug 7, 2024

@hi-ogawa Thanks so much for the explanation. Passing the plugins through sounds good and intuitive to me. Looking forward to it.

For the vitest.config.mjs in the env, yes, in this case it does use a config file. However, that's just a way we provide to the env author to custom something easily in the last mile. And it eventually comes to inline config and will be called by startVitest() programatically. So I understand the issue is still the same.

The extra config file is not in the root of the component dir. I understand in such a case configFile: true still doesn't work, right? And the config file could be anywhere in the dev env or another dependency package. So to avoid unexpected effects we disabled configFile and envFile by default.

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Aug 7, 2024

The extra config file is not in the root of the component dir. I understand in such a case configFile: true still doesn't work, right?

@Jinjiang I was thinking of doing configFile: configPath at L117 https://bit.cloud/teambit/vite/vitest-tester/~code/utils.ts#l117 and removing mergeConfig below since configPath: string seems always defined. Would it change something too much?

@sheremet-va
Copy link
Member

Passing the plugins through sounds good and intuitive to me.

I just want to say that it seems very counterintuitive to me. The config file with browser.enabled: true is the config with plugins field already.

@sheremet-va
Copy link
Member

But I think it would make sense if we applied viteOverrides to the browser project config if the workspace project reuses root config (meaning, there no custom workspaces)

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Aug 7, 2024

But I think it would make sense if we applied viteOverrides to the browser project config if the workspace project reuses root config (meaning, there no custom workspaces)

That might be okay, but the concern is same as a previous issue since that would cause plugins instances to be passed to two vite servers and that can break plugin hook convention (like buildStart runs multiple times).

@Jinjiang
Copy link
Author

Jinjiang commented Aug 7, 2024

The extra config file is not in the root of the component dir. I understand in such a case configFile: true still doesn't work, right?

@Jinjiang I was thinking of doing configFile: configPath at L117 bit.cloud/teambit/vite/vitest-tester/~code/utils.ts#l117 and removing mergeConfig below since configPath: string seems always defined. Would it change something too much?

Sure. I've tried it. I thought it was just an equivalent change (from load and merge to configFile) but it worked after that. I think we can take this for now. Thanks a lot.

@Jinjiang
Copy link
Author

Jinjiang commented Aug 7, 2024

Passing the plugins through sounds good and intuitive to me.

I just want to say that it seems very counterintuitive to me. The config file with browser.enabled: true is the config with plugins field already.

I personally feel the most confusing part to me is, with the same config, a file works and an inline object doesn't. The other approaches are also good to me.

Anyway, good to know the complication behind it through this issue. From the surface I just saw a new test.browser field.

Thanks.

@sheremet-va sheremet-va added p2-to-be-discussed Enhancement under consideration (priority) and removed pending triage labels Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Status: P2 - 2
Development

No branches or pull requests

3 participants