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

Allow prefixing the generated path #2254

Closed
wants to merge 1 commit into from
Closed

Allow prefixing the generated path #2254

wants to merge 1 commit into from

Conversation

Grafikart
Copy link

@Grafikart Grafikart commented Feb 25, 2021

This will allow working with vite with any backend.

It's not possible to inject a full path with the config since the full URL is transformed by resolveBaseUrl()

{
    base: 'http://localhost:3000/assets/',
}
// Will set config.path = "/assets/"

So every path generated by fileToDevUrl() will be /assets/... instead of http://localhost:3000/assets/.

With this fix we can use

VITE_BASE=http://localhost:3000 yarn run dev

And the generated paths will contain the full path (so vite can be used with any server) and fix #2196

This will allow working with vite with a backend.

It's not possible to inject this with the config since the full URL is transformed by resolveBaseUrl()

```js
{
    base: 'http://localhost:3000/assets/',
}
```

With this fix we can use 

```
VITE_BASE=http://localhost:3000 yarn run dev
```

And the generated paths will contain the full path (so vite can be used with any server) and fix #2196
@innocenzi
Copy link
Contributor

innocenzi commented Feb 26, 2021

While this fix would work, I think the fact that this feature is meant to ease the integration with back-ends should make us consider to use a configuration option instead, so plugins can easily update it.

Alternatively, a hook like proposed in #1675.

@Grafikart
Copy link
Author

@innocenzi The PR you linked is for build time right ? Cause we need this for the devServer too, I could add a new configuration option but I have no idea how to name it :D

@innocenzi
Copy link
Contributor

Yeah it's for the build-time URL. But that was just an idea, it's probably overkill. It makes sense for the build options to be highly flexible, but for the development URLs, I think a configuration option is the best bet.

I was going to suggest something like server.base or server.host but both are used. How about a top-level devBase or something similar?

@jonaskuske
Copy link
Contributor

Why do we need a separate BASE environment variable here instead of adding support to config.base, so both of these are valid?

{ base: '/site/' }

and

{ base: 'http://localhost:3000/site/' }

@SirDavidLudwig
Copy link

SirDavidLudwig commented Feb 26, 2021

@jonaskuske Well, since the vite.config file should be consistent among developers, using a direct config option can limit the flexability on the developers in the dev server. For instance, I use Valet which allows me to make *.test URLs to my website, but other devs may not have this. Without an environment variable, we would have to configure our config files differently which would break version control. And so it would then be highly beneficial for each developer to able to configure their own URL for the dev server. Then the config.base option can hold up for production builds.

@jonaskuske
Copy link
Contributor

jonaskuske commented Feb 26, 2021

@SirDavidLudwig you could just use the environment variable in your Vite Config¹ and pass that as base, or use the CLI and start the dev server with yarn dev --base http://mysite.test/, then there's no need to modify the config file for individual devs.

I think having two separate base options that are defined in different places and then stitched together is quite confusing 😅


¹ note that you currently need to call loadEnv explicitly in the Vite config – by default, env variables aren't loaded when the config is resolved: #2260

@SirDavidLudwig
Copy link

SirDavidLudwig commented Feb 26, 2021

@jonaskuske Ah, can't believe I didn't think of that; that's actually a much better solution! (my brain ist still stuck in the realm of json configs I think maybe idk :p)

@innocenzi
Copy link
Contributor

innocenzi commented Feb 26, 2021

Why do we need a separate BASE environment variable here instead of adding support to config.base

I thought the way it works currently was intended, but if I'm wrong, this can be a better fix indeed.

you could just use the environment variable in your Vite Config and pass that as base

Usually I would disagree since it'd kind of kill the point of using an environment variable, but a lot of Vite's configuration is environment-specific (like server.host or server.port).

I think a configuration like this would be ideal:

export default defineConfig({
  base: process.env.ASSET_URL
})

Otherwise, an additional top-level option, or an environment variable that would not be part of the config at all.

EDIT: Ah, I responded too late. Woops

@innocenzi
Copy link
Contributor

@jonaskuske Do you know what is this for? Is it useful to strip external URLs' hosts in development mode? I think just that would be enough to add support like you suggested.

@SirDavidLudwig
Copy link

SirDavidLudwig commented Mar 10, 2021

The solution in this PR is definitely not enough to fix this issue as other plugins will still potentially suffer (for example, this doesn't work with Vue files).

After some experimentation, I've managed to come up with a different solution that should be plugin-independent, and so far has worked with Vue and CSS. While I'm sure my solution can be improved upon, I thought I'd explain my process to see if it would at least give a better idea at what needs to really be changed to make this work.

This fix requires two small modifications across two different functions.

  1. The first modification prevents the external URL provided to the base config option from being stripped away when using the dev server. This is accomplished by replacing the if statement if statement here with the following:
if (!isExternalUrl(base)) {
    // ensure leading slash
    if (!base.startsWith('/')) {
        logger.warn(
            chalk.yellow.bold(`(!) "base" option should start with a slash.`)
        )
        base = '/' + base
    }
}
  1. The next modification fixes the URLs generated in the Vite client script. The clientInjectionsPlugin function merges the server's port and base into a single string for __HMR_PORT__ which breaks asset loading since it creates a kind of doubled URL. This issue addressed by replacing the if statements here to first check if hmrBase is an external URL, otherwise do the normal processing. The resulting if statement is shown below.
if (isExternalUrl(hmrBase)) {
    port += "/";
} else {
    if (options.path) {
        hmrBase = path__default.posix.join(hmrBase, options.path);
    }
    if (hmrBase !== '/') {
        port = path__default.posix.normalize(`${port}${hmrBase}`);
    }
}

This solution yields only a single minor issue that I can see which is the displayed local and network addresses the server is running on as it doubles up the URL since it's merging the server's URL with the external URL base.

EDIT:
I just wanted to add, you can make these changes much more manageable by generating patch-files using patch-package. Make the changes once, and everywhere else you use the repo the patches will automatically be applied.

@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Mar 23, 2021
@jrmyio
Copy link

jrmyio commented Jun 22, 2021

@SirDavidLudwig I couldn't access the files directly because of an:
The server is configured with a public base URL of... -error

So I commented-out these lines:
https://github.com/vitejs/vite/blob/main/packages/vite/src/node/server/middlewares/base.ts#L30-L36

Getting uglier and uglier :(

@antfu
Copy link
Member

antfu commented Jun 26, 2021

I don't think it's a good pattern to have vite replying solely on a magical env but not config. Closing this PR for now, and feel free to bring up other solutions in another thread. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

base option does not support external URLs with the development server
7 participants