fix: inline proxies for virtual html in transformIndexHtml #7993

saurabhdaware · 2022-05-02T17:14:49Z

Description

When inlined css is passed through the vite.transformIndexHtml it was failing with isSelfAccepting of undefined error on import-analysis plugin.

I added an early null return there which seem to fix the issue. Although I am new to Vite's codebase so I am not sure if this is the right fix.

Let me know if something else is a fix.

Additional context

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines.
Read the Pull Request Guidelines and follow the Commit Convention.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Ideally, include relevant tests that fail without this PR but pass with it.

patak-dev · 2022-05-02T18:56:47Z

I don't know if this is the correct fix. As you showed in #6859 (comment), the inline <style> is causing this issue. Removing it there or in this reproduction fixes the problem. I think we introduced this bug here: #7869.

@poyoho, we're probably missing an SSR + <style> test case like the one @saurabhdaware is showing here. I think the problem is we're calling transform without the id/importer being first added to the module graph. But IIUC, we shouldn't return null if it is undefined, but transform it without updating the metadata.

patak-dev · 2022-05-02T20:46:15Z

In the repro at #6859 (comment), url is /, so filename ends up being the project root folder.

vite/packages/vite/src/node/server/middlewares/indexHtml.ts

Line 193 in 1db7c49

const url = filename + `?html-proxy&${index}.css`

And then it fails to add the module to the graph because it can resolve the URL to add the module. I don't know how to solve this. Don't we also have the same issue for inlined JS modules @poyoho?

poyoho · 2022-05-03T03:00:46Z

I think we should do it in the source. @saurabhdaware I think we should add the test case to let errors be found in CI.

may be we can resolveId like server entry.

vite/packages/vite/src/node/server/transformRequest.ts

Lines 143 to 144 in ac05f25

    
           const id = 
        
             (await pluginContainer.resolveId(url, undefined, { ssr }))?.id || url

or rewrite the url to truth?

poyoho · 2022-05-03T04:27:13Z

in ssr run resolveId will let resolve plugin run, so it can resolve the package.json main field.

inlined JS modules are requested by <script type=module src='index.html?html-proxy&0.js'> the URL seems to handle it here.

vite/packages/vite/src/node/server/transformRequest.ts

Lines 143 to 144 in ac05f25

    
           const id = 
        
             (await pluginContainer.resolveId(url, undefined, { ssr }))?.id || url

IIUC it may be no issues in the inline js module.

poyoho · 2022-05-03T04:31:54Z

@saurabhdaware and I suggest no make vite resolveId with packge.json, I think you can make the url readable before exec vite.transformIndexhtml. I think the / rewrite to /index.html is better than /server.js.

For example:

vite/packages/playground/ssr-html/server.js

Lines 47 to 53 in 2a764af

    
           let [url] = req.originalUrl.split('?') 
        
           if (url.endsWith('/')) url += 'index.html' 
        
           const htmlLoc = resolve(`.${url}`) 
        
           let html = fs.readFileSync(htmlLoc, 'utf8') 
        
           html = html.replace('</body>', `${DYNAMIC_SCRIPTS}</body>`) 
        
           html = await vite.transformIndexHtml(url, html)

saurabhdaware · 2022-05-03T05:50:43Z

aha understood! This one did fix both the issues (#7992, #6859 (comment)

 let [url] = req.originalUrl.split('?') 
 if (url.endsWith('/')) url += 'index.html'

Let me pull this branch and see if it is working for my case. I'll see if we can add test for this. Thanks @poyoho 🎉

saurabhdaware · 2022-05-03T07:03:52Z

So this didn't fix the issue 2a764af

This line itself is failing on unable to resolve error.

vite/packages/vite/src/node/server/middlewares/indexHtml.ts

Line 195 in 2a764af

(await server!.pluginContainer.resolveId(filename))?.id || filename

saurabhdaware · 2022-05-03T07:04:42Z

ccda81b this fixed the issue but it still prints that error in terminal. Only the server started working after this.

saurabhdaware · 2022-05-03T07:22:11Z

a2a0d4f after this too the page started loading but the terminal was still throwing the error 🤔

saurabhdaware · 2022-05-03T07:29:20Z

packages/playground/ssr-html/server.js

@@ -44,13 +52,18 @@ async function createServer(

  app.use('*', async (req, res) => {
    try {
-      let [url] = req.originalUrl.split('?')
-      if (url.endsWith('/')) url += 'index.html'
+      const url = req.originalUrl


Updated this code to be a bit similar to SSR docs example

This one is failing with unresolved path error now

#7993 (comment) this can solve the issue. Is that expected behaviour?

Was wondering what exactly is the solution-

Update docs to add the solution that @poyoho mentioned

Or fix the resolve logic and automatically handle index.html paths?

We can update the docs in v3 if we think that is the correct way forward, but this was working before and documented so we should merge a fix in 2.9 that works as it is currently documented:

const url = req.originalUrl

You can watch resolve plugin to know how vite internal resolveId work. So you can set the package.json>main field to avoid the error.

@poyoho do you see an issue with adding this logic:

let [url] = req.originalUrl.split('?') if (url.endsWith('/')) url += 'index.html'

to transformIndexHtml?

Or would it later fail in render (https://vitejs.dev/guide/ssr.html#setting-up-the-dev-server)

// 4. render the app HTML. This assumes entry-server.js's exported `render` // function calls appropriate framework SSR APIs, // e.g. ReactDOMServer.renderToString() const appHtml = await render(url)

Since we are asking users to add the index.html by hand, why not add it ourselves when needed so we avoid breaking these cases? Maybe we could emit a warning later saying that in v3 this will not be supported and they have to pass a complete path?

saurabhdaware · 2022-05-03T08:33:39Z

So why do we exactly need filename?

and wouldn't this cause issues in SSR where someone is doing this-

app.get('/', (req, res) => {
  const html = `
  <html>
  <head>
    <style>
    body {
      color: red;
    }
    </style>
  </head>
  <body>
    index
  </body>
  </html>
  `

  const finalHtml = vite.transformIndexHtml(req.url, html);
  res.status(200).end(finalHtml)
});

In this case, there won't be any "filename" right? the HTML is generated on the runtime. How do we handle this in vite?

poyoho · 2022-05-03T09:41:22Z

my mean is add this before vite.transformIndexHtml let the url to be a file no a folder. or if the url is a folder, it will run the resolve plugin load the package.json use main field and make it to be a file instead of folder.

 let [url] = req.originalUrl.split('?') 
 if (url.endsWith('/')) url += 'index.html' 

vite.transformIndexHtml(url, `html`)

I think vite.transformIndexHtml('/', 'html') is an error call code, so we just need to add the docs on how to avoid it?

patak-dev · 2022-05-03T10:19:50Z

@brillout maybe you could help us here, would you consider that this isn't supported by Vite? So this isn't a regression?

vite.transformIndexHtml('/', template)

Several of our playgrounds are doing the resolution to add index.html before passing it to transformIndexHtml:

 let [url] = req.originalUrl.split('?') 
 if (url.endsWith('/')) url += 'index.html'

The SSR docs don't include this though https://vitejs.dev/guide/ssr.html#setting-up-the-dev-server, so there may be people passing \ to transformIndexHtml. Following the code, it doesn't seem that this was intended to be supported.

@poyoho maybe for 2.9, should we detect that \ was passed to transformIndexHtml and in that case avoid transforming the inline style tags and leave them as is? We could even add a warning about this in that case. So this isn't a breaking change if people followed the docs as is.

Not related to this PR, but the transformation of inlined style tags and inline JS, we don't have an SSR flag in transformIndexHtml, so these are transformed as if we weren't in SSR mode. I think there could be bugs hiding here

brillout · 2022-05-03T12:25:47Z

vite-plugin-ssr 0.3 does vite.transformIndexHtml('/', template) if the URL is /.

The only thing vite-plugin-ssr needs from transformIndexHtml() is the injection of the HMR scripts.

That's why I changed vite-plugin-ssr 0.4 to pass a fake HTML to transformIndexHtml():

const fakeHtmlBefore = '<html><head></head><body></body></html>'
// vite-plugin-ssr 0.4 always sets the URL to `/` even if the URL is different. (Vite doesn't
// seem to use the URL for that fake HTML.)
const fakeHtmlAfter = await pageContext._viteDevServer.transformIndexHtml('/', fakeHtml)

const scriptTagsInjectedByVite = extractScriptTags(fakeHtmlAfter)
// Insert script tags into the real HTML
// ...

In the context of SSR frameworks, I don't see a use case for inline styles HMR. (Some CSS-in-JS library do inject style tags to the SSR'd HTML but these don't need HMR.)

@saurabhdaware What's your motivation for using inline style HMR together with SSR? Why not import './some.css' instead?

I don't why Vite wants the URL (I've always wondered that actually). But I don't think it's needed for SSR?

Long term, maybe the solution is to have a second hook transformSsrHtml. (The name "transformIndexHtml" makes sense for a catch-all-URL SPA but doesn't in the context of SSR.)

// Doesn't support inline styles HMR, and therefore doesn't require a URL.
export function transformSsrHtml(
  html: string,
  // `transformIndexHtml()` also injects the base URL, but vite-plugin-ssr doesn't need it and
  // it actually causes problems. That's why vite-plugin-ssr uses this trick of passing a fake
  // HTML to Vite. I actually wonder if any SSR framework needs it?
  options?: { injectBaseUrl?: boolean }
) {
  // ...
}

Btw. is there a timeline for Vite 3? I think it would be good if we can have some design discussions. (My biggest pet peeve being that Vite doesn't externalize all SSR dependencies by default :-).) At least I think we should list all design flaws we are aware of and that we all agree on. So that we can, step-by-step, go in the right direction. Knowing that the design was agreed on, makes PR'ing much more pleasant. (I can take a stab at a couple of things but let me release Vike & Stem before :-), it's been quite delayed already 😅).

saurabhdaware · 2022-05-03T12:34:49Z

That's why I changed vite-plugin-ssr 0.4 to pass a fake HTML to transformIndexHtml():

Aha this seems like a nice workaround.

@saurabhdaware What's your motivation for using inline style HMR together with SSR? Why not import './some.css' instead?

I don't really mind not having HMR on inlined styles. We can just treat it as any other change in the html file right? and just reload the page?

In my case, I am working on an SSG so the <script src=""> and <style> can be added by the consumer in the template.

patak-dev · 2022-05-03T12:36:41Z

vite-plugin-ssr 0.3 does vite.transformIndexHtml('/', template) if the URL is /.

Thanks for the explanation, this confirms that we have a regression. We can fix it in v2.9, and then think about transformSsrHtml for v3. I think a PR or issue with this info would be good to keep that moving

Btw. is there a timeline for Vite 3? I think it would be good if we can have some design discussions. (My biggest pet peeve being that Vite doesn't externalize all SSR dependencies by default :-).) At least I think we should list all design flaws we are aware of and that we all agree on. So that we can, step-by-step, go in the right direction. Knowing that the design was agreed on, makes PR'ing much more pleasant. (I can take a stab at a couple of things but let me release Vike & Stem before :-), it's been quite delayed already 😅).

We have an open discussion here:

Vite 3 #7611.

If you want to focus on SSR for v3, you can choose to add a thread there, or start a new discussion and link it to the main issue.

About the timeline, once this regression is resolved, we are going to start merging the PRs in the v3 milestone. I think we could stay in beta longer for the major so we have more time for changes, so probably v3 would be released in 40-60 days.

patak-dev · 2022-05-03T21:50:17Z

packages/playground/ssr-html/server.js

+      // Force calling transformIndexHtml with url === '/', to simulate
+      // usage by ecosystem that was recommended in the SSR documentation
+      // as `const url = req.originalUrl`
+      const html = await vite.transformIndexHtml('/', template)


I'm forcing '/' as the url here as @brillout described he was doing in vite-plugin-ssr.

patak-dev · 2022-05-03T21:55:29Z

The PR is now using virtual paths for the proxies when the HTML is virtual. This fixes issues with sourcemaps (I think we may be able to remove the maybeVirtualHtmlSet @sapphi-red after we merge it, but leaving that for a future PR).
@saurabhdaware @poyoho would you test that this is working fine for you?

patak-dev · 2022-05-03T21:57:11Z

packages/vite/src/node/server/middlewares/indexHtml.ts


      // ensure module in graph after successful load
      const mod = await moduleGraph.ensureEntryFromUrl(url, false)
      ensureWatchedFile(watcher, mod.file, config.root)

-      const result = await server!.pluginContainer.transform(code, url)
+      const result = await server!.pluginContainer.transform(code, mod.id!)


The transform hook expects the resolved id as second parameter

BTW, mod.id is /path-to/index.html?html-proxy&index=0.css 😀

patak-dev · 2022-05-03T21:58:33Z

packages/vite/src/node/server/middlewares/indexHtml.ts

@@ -190,13 +217,13 @@ const devHtmlHook: IndexHtmlTransformHook = async (

  await Promise.all(
    styleUrl.map(async ({ start, end, code }, index) => {
-      const url = filename + `?html-proxy&${index}.css`
+      const url = `${proxyModulePath}?html-proxy&index=${index}.css`


The url was missing index= here, I think it worked because this is only needed as a temporary name but still good to follow the same convention as with JS I think.

packages/vite/src/node/server/middlewares/indexHtml.ts

saurabhdaware · 2022-05-04T10:41:14Z

It is working for me 🎉 Thanks people 👯

aleclarson · 2022-11-11T23:13:11Z

It feels like this PR was overloaded with extra stuff not mentioned in the merge commit description? It would've been better to do a separate PR or make sure to update the commit description before merging. 😅

Can someone sum up the stuff that isn't directly related to inlining JS/CSS paths for virtual HTML? Or maybe I'm just misunderstanding the point of this PR (no use case was stated in the PR description, so I'm left to guess what is meant by "inlined JS/CSS for virtual HTML").

saurabhdaware added 2 commits May 2, 2022 22:41

fix(importAnalysis): early null return on undefined

1d34d51

chore: revert isDebug change

ac05f25

saurabhdaware changed the title ~~fix(import-analysis): early null return on undefined~~ fix(import-analysis): cannot set property isSelfAccepting of undefined May 2, 2022

saurabhdaware marked this pull request as ready for review May 2, 2022 17:26

fix: url may be '/' run resolveId get real path

2a764af

feat: add try catch to resolveId

ccda81b

feat: handle html name in getHtmlFilename

a2a0d4f

feat: update ssr-html example with SSR docs code

eb6bfc7

saurabhdaware commented May 3, 2022

View reviewed changes

fix: error

e5eedeb

poyoho and others added 3 commits May 3, 2022 21:24

fix: pass for hmr

cced917

chore: update, wip

b650fd6

fix: handle js and css proxies for virtual html

1bf08ff

patak-dev changed the title ~~fix(import-analysis): cannot set property isSelfAccepting of undefined~~ fix: inline proxies for virtual html in transformIndexHtml May 3, 2022

patak-dev reviewed May 3, 2022

View reviewed changes

patak-dev requested a review from poyoho May 3, 2022 21:51

patak-dev reviewed May 3, 2022

View reviewed changes

poyoho reviewed May 4, 2022

View reviewed changes

packages/vite/src/node/server/middlewares/indexHtml.ts Show resolved Hide resolved

poyoho approved these changes May 4, 2022

View reviewed changes

patak-dev merged commit d49e3fb into vitejs:main May 4, 2022

sapphi-red mentioned this pull request May 4, 2022

chore: remove maybeVirtualHtmlSet #8010

Merged

9 tasks

saurabhdaware deleted the fix/undefined-importermodule branch May 4, 2022 10:41

brillout mentioned this pull request May 21, 2022

feat: spa option, preview and dev for MPA and SSR apps #8217

Merged

9 tasks

justb3a mentioned this pull request Apr 28, 2023

Cannot read properties of undefined (reading 'isSelfAccepting') #13030

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: inline proxies for virtual html in transformIndexHtml #7993

fix: inline proxies for virtual html in transformIndexHtml #7993

saurabhdaware commented May 2, 2022

patak-dev commented May 2, 2022

patak-dev commented May 2, 2022

poyoho commented May 3, 2022

poyoho commented May 3, 2022

poyoho commented May 3, 2022

saurabhdaware commented May 3, 2022

saurabhdaware commented May 3, 2022

saurabhdaware commented May 3, 2022

saurabhdaware commented May 3, 2022

saurabhdaware May 3, 2022 •

edited

Loading

saurabhdaware May 3, 2022

patak-dev May 3, 2022

poyoho May 3, 2022

patak-dev May 3, 2022

saurabhdaware commented May 3, 2022

poyoho commented May 3, 2022 •

edited

Loading

patak-dev commented May 3, 2022

brillout commented May 3, 2022

saurabhdaware commented May 3, 2022

patak-dev commented May 3, 2022

patak-dev May 3, 2022

patak-dev commented May 3, 2022

patak-dev May 3, 2022

poyoho May 4, 2022

patak-dev May 3, 2022

saurabhdaware commented May 4, 2022 •

edited

Loading

aleclarson commented Nov 11, 2022

fix: inline proxies for virtual html in transformIndexHtml #7993

fix: inline proxies for virtual html in transformIndexHtml #7993

Conversation

saurabhdaware commented May 2, 2022

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

patak-dev commented May 2, 2022

patak-dev commented May 2, 2022

poyoho commented May 3, 2022

poyoho commented May 3, 2022

poyoho commented May 3, 2022

saurabhdaware commented May 3, 2022

saurabhdaware commented May 3, 2022

saurabhdaware commented May 3, 2022

saurabhdaware commented May 3, 2022

saurabhdaware May 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabhdaware commented May 3, 2022

poyoho commented May 3, 2022 • edited Loading

patak-dev commented May 3, 2022

brillout commented May 3, 2022

saurabhdaware commented May 3, 2022

patak-dev commented May 3, 2022

Choose a reason for hiding this comment

patak-dev commented May 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabhdaware commented May 4, 2022 • edited Loading

aleclarson commented Nov 11, 2022

saurabhdaware May 3, 2022 •

edited

Loading

poyoho commented May 3, 2022 •

edited

Loading

saurabhdaware commented May 4, 2022 •

edited

Loading