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

fix: respect top-level server.preTransformRequests #19272

Merged

Conversation

hi-ogawa
Copy link
Collaborator

@hi-ogawa hi-ogawa commented Jan 23, 2025

Description

It looks like there were two changes related to preTransformRequests on Vite 6, namely:

  • server.preTransformRequests has no effect on environment.dev.preTransformRequests.
  • while server.preTransformRequests: true by default both on client and ssr on Vite 5, now only client environment dev.preTransformRequests: true and ssr does not pre transform by default.

I made a reproduction in https://github.com/hi-ogawa/reproductions/tree/main/vitest-7345-preTransformRequests-false.

I assume the first one is a bug since currently server.preTransformRequests is no-op and we should either deprecate/remove or use it as a default. This is what this PR addresses and I confirmed this fixed Vitest's issue https://stackblitz.com/edit/bolt-vue-7wha3d5d?file=package.json. There are also some usages in the wild https://github.com/search?q=preTransformRequests&type=code.

For the 2nd one, I wasn't aware of this change and I'm not sure if it's intended. I'll leave this part out of the PR for now.

@hi-ogawa hi-ogawa force-pushed the fix-server-preTransformRequests-compat branch from 5c35f99 to 97d7f6e Compare January 24, 2025 09:36
@hi-ogawa hi-ogawa changed the title fix: backward compat for server.preTransformRequests fix: fix server.preTransformRequests Jan 24, 2025
@hi-ogawa hi-ogawa changed the title fix: fix server.preTransformRequests fix: fix preTransformRequests Jan 24, 2025
@hi-ogawa hi-ogawa changed the title fix: fix preTransformRequests fix: respect top-level server.preTransformRequests Jan 25, 2025
@hi-ogawa hi-ogawa marked this pull request as ready for review January 25, 2025 08:13
Copy link

pkg-pr-new bot commented Jan 25, 2025

Open in Stackblitz

npm i https://pkg.pr.new/vite@19272

commit: 0e056cc

@sapphi-red sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Feb 3, 2025
@sapphi-red
Copy link
Member

  • server.preTransformRequests has no effect on environment.dev.preTransformRequests.

Checking the reason why this options was introduced (#6309), maybe it makes sense to sync the value with environments.client.dev.preTransformRequests inside of the default value for all environments.*.dev.preTransformRequests? It feels like there won't be a case where you want to enable/disable it for all the environments.

  • while server.preTransformRequests: true by default both on client and ssr on Vite 5, now only client environment dev.preTransformRequests: true and ssr does not pre transform by default.

I didn't find any links, but I think this was a intentional change.

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Let's merge this fix for now. I also think the change to only pre transform for the client was intentional but I can't find now a reference for that. We should change that default in another PR if we think that is needed. For environments running in different processes, maybe we have the same peformance profile as with the browser requesting modules. I don't recall us doing profiling for different cases with and without preTransformRequests for SSR. But Vitest disables it for performance reasons IIRC.

@patak-dev patak-dev merged commit 12aaa58 into vitejs:main Feb 3, 2025
25 checks passed
@hi-ogawa hi-ogawa deleted the fix-server-preTransformRequests-compat branch February 3, 2025 23:30
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) trigger: preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to disable Pre-transform error log?
3 participants