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

resolveId is called before buildStart with enforce: 'pre' #5616

Closed
6 tasks done
sagargurtu opened this issue Apr 24, 2024 · 5 comments · Fixed by #5646
Closed
6 tasks done

resolveId is called before buildStart with enforce: 'pre' #5616

sagargurtu opened this issue Apr 24, 2024 · 5 comments · Fixed by #5646
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)

Comments

@sagargurtu
Copy link
Contributor

Describe the bug

buildStart() is not called for plugins with enforce: 'pre'.

Reproduction

https://stackblitz.com/edit/vitest-dev-vitest-gdhqm5

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.18.0 - /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.5.1 
    vite: latest => 5.2.10 
    vitest: latest => 1.5.1

Used Package Manager

npm

Validations

@hi-ogawa
Copy link
Contributor

It looks like buildStart is still called, but the issue is that resolveId is called before buildStart when you have enforce: "pre".
I also made a repro here:
https://github.com/hi-ogawa/reproductions/tree/main/vitest-5616-plugin-build-start

> vitest

@resolveId(pre) { source: 'vitest' }
@resolveId(pre) { source: 'vitest' }
@buildStart(pre)
@buildStart(normal)

 DEV  v1.5.1 /home/projects/wmaivylmj.github
...

I remembered someone mentioned that buildStart is not working, but it looks like the same issue where resolveId being getting called first:

@hi-ogawa hi-ogawa added p3-minor-bug An edge case that only affects very specific usage (priority) and removed pending triage labels Apr 25, 2024
@hi-ogawa
Copy link
Contributor

This resolveId seems to be the trigger, which is called during configureServer and before buildStart

const projectVitestPath = await this.vitenode.resolveId('vitest')

Quick logging from plugin shows something like this:

@configureServer(pre)
@resolveId(pre) { source: 'vitest' }
@resolveId(pre) { source: 'vitest' }
@configureServer(normal)
@buildStart(pre)
@buildStart(normal)

@sagargurtu Can you explain what is your use case? The linked discussion mentioned rollup-plugin-node-externals. I think this is technically what Vitest shouldn't do, but I'm not sure if there is a quick way around.
Depending on the use case, there might be a workaround on plugin side.

@Septh
Copy link

Septh commented Apr 25, 2024

Hi! rollup-plugin-node-externals author here.

I was not aware of this and the linked discussion until @chandu filed Septh/rollup-plugin-node-externals#30 which I fixed by not relying on buildStart to initialize my variables. Simple enough.

However I do believe that this is a bug that should be fixed in Vitest because the order in which the hooks are called is clearly defined by Rollup's documentation and diverging from that order is wrong (and my plugin might not be the only one affected).

@hi-ogawa hi-ogawa changed the title buildStart() is not called for plugins with enforce: 'pre' resolveId is called before buildStart with enforce: 'pre' Apr 25, 2024
@sagargurtu
Copy link
Contributor Author

@hi-ogawa While I could potentially move my initialization code to config() or the function that returns the plugin object, I agree with @Septh as many other existing plugins could break because of this issue.

@Septh
Copy link

Septh commented May 2, 2024

Awesome! Thanks, guys. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants