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: prevent double base in assets on dev mode when assets plugin is applied before html transformation #15345

Closed
wants to merge 1 commit into from

Conversation

m1san
Copy link
Contributor

@m1san m1san commented Dec 14, 2023

Description

On dev mode, for absolute asset URLs in monorepos, the assets plugin resolves the asset including the base and origin from the vite config when calling fileToDevUrl, then when transformIndexHtml is applied, the indexHtml plugin appends base again when resolving the asset path in processNodeUrl the base is appended again.

Fixes #15214

Before
Screenshot 2023-12-13 at 4 22 31 PM
Screenshot 2023-12-13 at 4 22 24 PM

After
Screenshot 2023-12-13 at 4 22 31 PM
Screenshot 2023-12-13 at 4 24 13 PM

Additional context

Setup
vite config

{
  base: '/my-path/'
}

directory structure

/
  /lib
  /app

example snippet
library-file.tsx

 import myicon from './myicon.svg'

 export const LibComponent = () =>  <img src={myicon} />;

app-file.tsx

import { LibComponent } '@namespace/lib';

export const appComponent = () => <LibComponent/>;

The resolved asset URL in dev mode results as /my-path/my-path/@fs/Users/myuser/example/lib/library-assets/myicon.svg


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, especially the Pull Request Guidelines.
  • 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).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Dec 14, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@m1san m1san changed the title Avoid duplicate base prefix applied on ssr when assets plugin applied… fix: prevent double base in assets on dev mode when assets plugin is applied before html transformation Dec 14, 2023
originalUrl !== '/' &&
htmlPath === '/index.html')
// avoid duplicate base prefix when applying html transforms after assets plugin
!url.startsWith(config.base) &&
Copy link
Member

Choose a reason for hiding this comment

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

We can't do this, or we won't support projects having /root-path/base/ in the fs.
/base/base/file.js is a valid module.

@patak-dev
Copy link
Member

Checking the reproduction, transformIndexHtml shouldn't be called on the result of render as documented in https://vitejs.dev/guide/ssr.html#setting-up-the-dev-server. It is intended to be called for the index.html template.

@m1san
Copy link
Contributor Author

m1san commented Dec 31, 2023

Checking the reproduction, transformIndexHtml shouldn't be called on the result of render as documented in https://vitejs.dev/guide/ssr.html#setting-up-the-dev-server. It is intended to be called for the index.html template.

@patak-dev In the reproduction, the intent is to demonstrate rendering content without having an actual index html so I can do a direct render pass and use the result to call transformIndexHtml, mainly what I'm trying to solve is a different result by doing apply plugins -> transform index vs transform index -> apply plugins.

While I could do my own custom transformIndexHtml to solve the issue for me, I'd prefer contribute with a solution for the ecosystem, other suggestions are welcome.

Thanks for everything y'all do in Vite, really love using it so far. Happy new year!

@patak-dev
Copy link
Member

Let's close this PR. We can keep #15214 open to discuss. In case it is helpful, there are efforts to integrate RSC and Vite that don't seem to be running into that issue. Maybe a good idea to check them out facebook/react#26926

@patak-dev patak-dev closed this Jan 8, 2024
mkilpatrick added a commit to yext/pages that referenced this pull request Feb 6, 2024
This reworks the exports of _server.tsx. According to
[this](vitejs/vite#15345 (comment))
transformIndexHtml should be called on the index.html (which is provided
automatically) and not the entire result of calling render (after the
server code is injected). It hasn’t actually posed a problem but I’d
like to get ahead of it and make sure we’re doing it the right way. It
really only affects local dev but it’s breaking since what’s exported
from _server.tsx has changed.

_server.tsx is now expected to export 3 things:
```
export const render = async (pageContext: PageContext<any>) => {
  const { Page, pageProps } = pageContext;

  return ReactDOMServer.renderToString(<Page {...pageProps} />);
};

export const replacementTag = "<!--YEXT-SERVER-->";

export const indexHtml = `<!DOCTYPE html>
    <html lang="<!--app-lang-->">
      <head></head>
      <body>
        <div id="reactele">${replacementTag}</div>
      </body>
    </html>`;
```

Specifically, `indexHtml` is now split out from the server `render`. It
also requires a `replacementTag` to know where the server html should be
injected into the indexHtml.

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants