transformIndexHtml hook params ctx add full request param

bigbossx commented 1 year ago

Description

I'm a little curious why the current ctx only retains the originalUrl instead of the complete req. it looks like it's technically possible.

for me if can access full req in transformIndexHtml hook is very useful. eg. I have a vite plugin and use transformIndexHtml hook to transform html and return it, just like

  name:'vite-plugin-xxxx',
  async transformIndexHtml(html, ctx) {
      const res = await axios.get("xxx") // I need like ctx.req.host or headers to submit a request, we use Whistle to proxy to dev, anyway, just show there is indeed we need.
      return replacer(html,res);
    }

I fully know that transformIndexHtml is only work in vite dev server mode

Alternative

No response

Additional context

If this is indeed a potential requirement, willing to submit a PR. thanks!

Validations

[X] Follow our Code of Conduct
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that request the same feature to avoid creating a duplicate.

bluwy commented 1 year ago

The likely reason is that the entire req object isn't entirely related to transforming an html. Usually you'd only need to know which path to the html you're transforming. The request related information are kept for server middlewares instead.

So I'm not sure if it fits to expose the entire req here. Would the config values from resolvedConfig.server.host and resolvedConfig.server.headers be enough for your case?

mxxk commented 1 year ago

@bluwy wanted to contribute another use case which may motivate exposing req and res as command-specific properties on ctx passed to transformIndexHtml... I'm working on a CSP plugin for Vite, which has the following minimal example of generating CSP nonces (https://stackblitz.com/edit/vitejs-vite-pckfyp?file=vite.config.js):

let lastNonce;

const cspNonceDevPlugin = {
  name: 'csp-nonce',
  apply: 'serve',

  configureServer(server) {
    server.middlewares.use(
      util.callbackify(async (req, res) => {
        lastNonce = (await randomBytes(16)).toString('base64url');
        res.setHeader(
          'Content-Security-Policy',
          `script-src 'self' 'nonce-${lastNonce}'`
        );
      })
    );
  },

  transformIndexHtml(html) {
    const jsdom = new JSDOM(html);
    for (const script of jsdom.window.document.querySelectorAll('script')) {
      script.nonce = lastNonce;
    }
    return jsdom.serialize();
  },
};

Two drawbacks with the above implementation:

Since the middleware does not know whether this request is for an HTML file, it ends up applying Content-Security-Policy for all requests, not just HTML ones, even though Content-Security-Policy doesn't have any effect on, for example, image responses.

If there was a way to access res within transformIndexHtml, the middleware logic can be moved there.

Since Vite already does the work to determine whether a request is for an HTML file,

https://github.com/vitejs/vite/blob/0fed2104ad07f0de4dbf29ed8d6047b6918c7db3/packages/vite/src/node/server/middlewares/indexHtml.ts#L379-L401

reimplementing the same within middleware doesn't seem like a good solution.
Since there is no way to ensure that middleware and transformIndexHtml correspond to the same request, passing data using a global variable (lastNonce) seems unsafe. What if async requests are interleaved?
1. Request A (url: /) calls async middleware and sets lastNonce
2. Request B (url: /@vite/client) calls async middleware and overwrites lastNonce
3. Request A calls transformIndexHtml, which consumes lastNonce from (ii) instead of (i)
Even though req.originalUrl in middleware corresponds to ctx.originalUrl in transformIndexHtml, not all requests are HTML requests, so storing data keyed by req.originalUrl which is never consumed by transformIndexHtml also seems unworkable.

Circling back to the original ask, since transformIndexHtml already exposes context-specific parameters

https://github.com/vitejs/vite/blob/0fed2104ad07f0de4dbf29ed8d6047b6918c7db3/packages/vite/src/node/plugins/html.ts#L926-L939

it doesn't seem like such a bad idea to also expose req and res in vite dev and vite preview modes. WDYT?

bluwy commented 1 year ago

The context provided in transformIndexHtml today balances how you transform the HTML during dev and build, which in dev you have access to server and originalUrl, in build you have access to bundle and chunk.

req and res are geared much towards dev, and it could be misleading that req and res would also be set when you run your app in prod, which they don't.

Since the middleware does not know whether this request is for an HTML file, it ends up applying Content-Security-Policy for all requests, not just HTML ones

You can inject a post-middleware (return a function from configureServer) and check for req.url?.endsWith('.html') and it should also work. The indexHtml plugin you linked runs near the post-phase too.

Since there is no way to ensure that middleware and transformIndexHtml correspond to the same request, passing data using a global variable (lastNonce) seems unsafe. What if async requests are interleaved?

I don't have much experience with CSP. But if Vite does provide req and res today, how would you handle them in prod too? If prod does have a CSP middleware to handle it, I think it could also only be a middleware in a Vite plugin.

e.g. you can patch res.end = () => { /* do something and call res.end again */ } and inject your CSP handling there too.

(Also to be clear, I'm not opposed to adding it yet. Trying to understand its usecase as transformIndexHtml also has many caveats today)

mxxk commented 1 year ago

Thanks for getting back @bluwy!

req and res are geared much towards dev, and it could be misleading that req and res would also be set when you run your app in prod, which they don't.

Indeed, and my hope is that documentation could remediate any confusion about which parameters are available during which commands. Presently, this is covered in the documentation in the sentence

The context exposes the ViteDevServer instance during dev, and exposes the Rollup output bundle during build.

If any additional context-specific parameters were to be added, they would be optional and it could hopefully be clear which command they apply to. (Also, happy to contribute PR(s) to help with this if the time comes.) 😃

You can inject a post-middleware (return a function from configureServer) and check for req.url?.endsWith('.html') and it should also work. The indexHtml plugin you linked runs near the post-phase too.

Nice! Since this relies on post-middleware being run for HTML file requests, I hope that after #14690 is resolved by #14756, vite preview does the same; otherwise, this approach won't work for vite preview.

I don't have much experience with CSP. But if Vite does provide req and res today, how would you handle them in prod too? If prod does have a CSP middleware to handle it, I think it could also only be a middleware in a Vite plugin.

I wouldn't handle them in prod! 😅 At least for the approach I've taken with CSP, there are two distinct plugins which implement CSP differently—an apply: 'serve' plugin which uses nonces (useful in dev), and an apply: 'build' plugin which computes JS hashes (useful to minimize transformation in production server), injects integrity attributes to <script> tags, and emits a full set of hashes to a separate file (to be consumed by the production server). This separation seems to make sense because during vite dev, Vite is doing the serving, but in vite build, the serving is done by the production server (e.g., Express, GitHub Pages, etc.), so there is a natural asymmetry around which entity is responsible for emitting HTTP response headers in either case.

But if one is forced to use use nonces in production, I believe the plugin must emit a placeholder during build in transformIndexHtml (e.g., <script nonce="__NONCE_PLACEHOLDER__">)—which requires only the html parameter; no other context required. When the Vite build output is served to customers, the HTML files must be transformed at serve time to inject the nonce generated by the server.

e.g. you can patch res.end = () => { / do something and call res.end again / } and inject your CSP handling there too.

This is an interesting hack/patch! Will need to try it.

(Also to be clear, I'm not opposed to adding it yet. Trying to understand its usecase as transformIndexHtml also has many caveats today)

Thanks for clarifying your present feeling on the matter! I believe I may have run into one such caveat with transformIndexHtml in my implementation of the apply: 'build' plugin which computes hashes... As it turned out, transformIndexHtml runs before some final JS transformations during build—namely postImportMapHook():

https://github.com/vitejs/vite/blob/055d2b86b0543a7c1a2a4d5bc7298af62bc51fa7/packages/vite/src/node/plugins/html.ts#L288-L293

As a result, if the JS chunk referenced from HTML were to be hashed in transformIndexHtml, its content would be different from the final output, and therefore the hashes wouldn't match. To work around this, the plugin uses a generateBundle hook with order: 'post' to defer hashing JS chunks until after postImportMapHook() runs.

Zooming out a bit, the original thought behind exposing req and res within transformIndexHtml was not to upset the balance between build and serve modes, but rather to attempt to improve the ergonomics of working with the current request/response during vite dev/preview only, without having any effect on vite build. By contrast, during build, Vite already runs the rich set of Rollup hooks, so the ergonomics of working with the current module/chunk/bundle already seem quite flexible and extensible by comparison (e.g., given the generateBundle example above).

bluwy commented 1 year ago

Thanks for the explanation! That clears things up and I can see how it's useful here now. One thing to also note with transformIndexHtml is that it doesn't work with most SSR frameworks at the moment. Vite plugins can only run at build-time (and transform html in build time). SSR only generates the html in runtime so its too late then. This has hit a lot of Vite plugins that want to support many frameworks today.

But even so, I think this could be worth considering. I'll put this on the team board for discussion, though our backlog is kinda packed so it might take a while. Feel free to send a PR if you'd like too, it seems like a simple change.

mxxk commented 1 year ago

One thing to also note with transformIndexHtml is that it doesn't work with most SSR frameworks at the moment. Vite plugins can only run at build-time (and transform html in build time). SSR only generates the html in runtime so its too late then. This has hit a lot of Vite plugins that want to support many frameworks today.

Good to know, thanks for the callout! The SSR example made me think of something I didn't consider before—if req/res are added to the set of parameters passed in ctx of transformIndexHtml hook, what should happen when the hook is invoked via ViteDevServer.transformIndexHtml? On the one hand, ViteDevServer.transformIndexHtml could be extended to take req/res pass them through to transformIndexHtml hook, treating them as opaque values which default to undefined. On the other hand, it might be making an assumption about the SSR server being Express-like. What do you think?

From my perspective, the former seems to minimize any asymmetry, and may make sense if ViteDevServer.transformIndexHtml is primarily used for SSR in development mode.

But even so, I think this could be worth considering. I'll put this on the team board for discussion, though our backlog is kinda packed so it might take a while. Feel free to send a PR if you'd like too, it seems like a simple change.

Thanks again! 🙌 I would love to make a PR for this.

bluwy commented 1 year ago

if req/res are added to the set of parameters passed in ctx of transformIndexHtml hook, what should happen when the hook is invoked via ViteDevServer.transformIndexHtml?

I think it should be fine as the middlewares Vite provides in SSR are connect-like (requires req and res), so setting up a SSR framework like hapi with Vite would already not work well.

But it is something also think about though, the added req and res parameters passed to ViteDevServer.transformIndexHtml would not look great, that makes it 5 params. Unless maybe the third parameter is made an optional object.

mxxk commented 12 months ago

I think it should be fine as the middlewares Vite provides in SSR are connect-like (requires req and res), so setting up a SSR framework like hapi with Vite would already not work well.

Got it, thank you. 👍

But it is something also think about though, the added req and res parameters passed to ViteDevServer.transformIndexHtml would not look great, that makes it 5 params. Unless maybe the third parameter is made an optional object.

Took your advice and in #14797 made the last parameter an optional object.

mxxk commented 11 months ago

@bluwy now that Vite 5.x has been released (which I see you served as the release process lead for 🎉) I wanted to circle back on this issue and see if it ever made it to the Vite team discussion? Or perhaps it keeps getting displaced by higher-priority items? Not sure how frequently the team meets, so any visibility into this would be fantastic!

bluwy commented 10 months ago

Hey, yeah we haven't got to this yet. We usually meet once every 2 weeks, and got more progress than usual though, but it'll take a bit while to reach this. I'll set a proper label for this so it's more visible. Sometimes it gets hidden without it.

There was also some indirect discussion about making Vite less-dependant on connect-style API, so this would be another factor for the feature request.

vitejs / vite