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: Allow custom URL prefix in CSS #4337

Closed
wants to merge 6 commits into from
Closed

fix: Allow custom URL prefix in CSS #4337

wants to merge 6 commits into from

Conversation

jianqi-jin
Copy link
Contributor

@jianqi-jin jianqi-jin commented Jul 21, 2021

Description

this PR is aimed to fixed #3070/ #3646/ #1539..
There seems no way to custom the prefix of url (that like PNG/font/SVG etc) in CSS files. But also this feature maybe is very important in some cases(example the files server host is not same as the dev server's, especially when the application can not run at the local dev server).
So, this PR added the option named publicPath in CSS config to allow users to custom the prefix of url.

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.

@Shinigami92 Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label Jul 21, 2021
docs/config/index.md Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved
@Shinigami92 Shinigami92 added p2-nice-to-have Not breaking anything but nice to have (priority) and removed p3-minor-bug An edge case that only affects very specific usage (priority) labels Jul 21, 2021
@Shinigami92
Copy link
Member

I moved this to p2, cause it adds a new option

@Shinigami92
Copy link
Member

@patak-js Do you think we need a test for the new option?
At least that could help to check that this option will not accidentally removed by someone later on

@patak-dev
Copy link
Member

Tests are needed but could be delayed until there is approval for this feature.

The last time this was discussed, it looks like Evan said that this is not something that Vite will support #1539 (comment)

It would be good to see if this can't be done by a plugin in user land instead of adding more complexity in Core.

Shinigami92
Shinigami92 previously approved these changes Jul 21, 2021
Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

From my side, I give it an approval, cause it is just a simple if condition
Looks not very complex to me 🤔
But yes, we should take this to the next team meeting

@Shinigami92 Shinigami92 self-requested a review July 23, 2021 13:56
@Shinigami92 Shinigami92 dismissed their stale review July 23, 2021 13:57

We need to find another way. Info will come later.

@Shinigami92
Copy link
Member

The current implementation is a no-go, cause it would break to much.

But we discussed that we would like to change behavior with a new option e.g. here

return config.base + rtn.replace(/^\//, '')

This may only be on serve

Alternatively could be an option to force including hostname in asset paths

@jianqi-jin
Copy link
Contributor Author

jianqi-jin commented Jul 25, 2021

The current implementation is a no-go, cause it would break to much.

But we discussed that we would like to change behavior with a new option e.g. here

return config.base + rtn.replace(/^\//, '')

This may only be on serve
Alternatively could be an option to force including hostname in asset paths

Option 1 seems good for me, because my current project is Migrating from Webpack to Vite. If option 2 means modifying asset paths all reference, the workload is a little heavy, case when the HTML which has inline style sheet service is based on the back end, the asset is based on the local service (Vite) and has a different hostname.

For option 1, I should move the new config option from css.publicPath to server.publicPath, right?

By the way, does the config base option have a plan that supports complete and different hostnames?

if (isExternalUrl(base)) {
if (!isBuild) {
// get base from full url during dev
const parsed = parseUrl(base)
base = parsed.pathname || '/'
}

let hmrBase = config.base
if (options.path) {
hmrBase = path.posix.join(hmrBase, options.path)
}
if (hmrBase !== '/') {
port = path.posix.normalize(`${port}${hmrBase}`)
}

@innocenzi
Copy link
Contributor

The last time this was discussed, it looks like Evan said that this is not something that Vite will support

I think Evan was not aware that this is not easily doable in a lot of contexts

Personally, I just need to have the full base (protocol + domain + port) in the asset URL, as suggested by this solution.

@Shinigami92's proposal of a boolean option that would do that sounds good to me and would solve all of the back-end issues we have with Laravel, Craft, Symfony, Adonis, etc. We just need that in development, not while building.

@jianqi-jin
Copy link
Contributor Author

Modifying base looks like a big change...

@khalwat
Copy link
Contributor

khalwat commented Jul 26, 2021

I echo @innocenzi 's sentiments, it would solve the problems I'm having, too, and eliminate the need for custom plugin work-arounds ala: https://nystudio107.com/blog/using-vite-js-next-generation-frontend-tooling-with-craft-cms#vite-processed-assets

ref: #2394

@patak-dev
Copy link
Member

Thanks for your input @innocenzi @khalwat, I think a new assetsFullPathInDev (or assets.fullPathInDev... it is really hard to name this one, we may need to discuss again before merging) should also cover the case that @Akryum had last week.

The idea is to modify this line

return config.base + rtn.replace(/^\//, '')

with

return ( config.assetsFullPathInDev ? config.server.origin : '' ) + config.base + rtn.replace(/^\//, '')

Where server.origin is defined as ${protocol}://${hostname}:${port}/ and it is a new entry in the resolved config. I don't know if origin is the best name for protocol+host+port. Again, I don't know if this is the best name here. Maybe server.baseURL could also work?
This information is known at this point after starting the server

printServerUrls(hostname, protocol, serverPort, base, info)
, we may need to mutate the resolved config because there is the discovery of a free port (if --stricPort is not used). We are already doing the same with the port mutating it here
serverConfig.port = (httpServer.address() as AddressInfo).port

We may be able to get everything we need and set the origin or server base URL directly after setting the config port using httpServer.address().

@innocenzi
Copy link
Contributor

server.origin sounds good to me, as it is the definition of a protocol, host and port, according to Mozilla.

As for the boolean option I'd say something like assets.useOrigin or assets.useOriginInDev, but fullPathInDev seems fine too - though, for me, the "path" is everything after the origin, including a leading slash.

Either way, I'm looking forward to this issue being solved, thanks for working on it! :)

@khalwat
Copy link
Contributor

khalwat commented Jul 26, 2021

FWIW, I believe webpack uses the name public for this purpose. Where it gets complicated is in setups with a backend like Laravel or Craft CMS, there are two servers involved.

To distinguish them, the nomenclature I've been using is:

serverPublic - the URL the webserver is running off of

devServerPublic - the URL the dev webserver is running off of

https://github.com/nystudio107/craft-vite/blob/v1/src/config.php#L39

But I think given that Vite has already defined these in server.host, server.port, etc. probably there would be quite a bit of justifiable push-back against changing these at this point.

Given that the URLs these affect are things stored in the /public dir in Vite, I think I like the idea of naming it:

server.public

...because it would affect asset URLs that would be normally coming from the /public dir. Or if you wanted to be more specific, it could be:

server.publicUrl

...to pair with the existing publicDir setting.

You may not even need a boolean flag btw... you could just have it default to the normal behavior if this setting is empty, and use the URL specified here if it is not.

@jianqi-jin
Copy link
Contributor Author

or,Can we change our thinking and add a plugin hook called resolveurl(or something)? I don't know if this has an impact on runtime performance.

@patak-dev
Copy link
Member

I also discussed with @Akryum and the boolean option would not be enough for him. As it's served on http://localhost:3000/ but the local site is on https://webpack.livestorm.local/
So maybe assets.originInDev could be of type boolean | string to accommodate that use case.

But what @khalwat points out shows that the requirements for this feature are not clear. If the real requirement is that everything from public is going to have the origin URL attached (or another local dev server URL), then it isn't the same as doing it for all asset types. @innocenzi your input would also be helpful here.

One problem with the proposed approach is that some URL are not touched by Vite. For example if you have a react component with some images pointing to public. There is some handling of this kind of paths in Vue templates when using the SFC compiler but we are going to be missing some public paths. So, the proposed change is not the same as the plugin you were using based on #2394 (comment). For reference

{
  enforce: 'pre',
  apply: 'serve',
  transform: (code, id) => {
    code = code.replace(/(from '|import\(')(\/src|~?@)\/(.*)\.(svg|png|mp3|mp4)/g, '$1https://webpack.livestorm.local/src/$3.$4?import=')
    code = code.replace(/(?<!local)(\/src|~?@)\/(.*)\.(svg|png|mp3|mp4)/g, 'https://webpack.livestorm.local/src/$2.$3')
    return {
      code,
      map: null,
    }
  },
}

Because with this plugin you are modifying all the URLs, not only the ones that Vite rewrites. The same applies to creating a new hook like @2359634711 proposed. I don't know how are we going to account for these URLs from inside Vite... looks like regex/replace is the only way to catch all (including some false positives)

I think that the cleanest way to do this would be to instruct the browser to do this redirect. Have you thought about using a service worker for this following the same pattern used in https://mswjs.io?
The idea would be that during dev, a plugin would be used that injects a SW that catches all assets requests or all public (or whatever combo you need) and redirects them to your other server, or adds the localhost (that it is available in the client IIUC). This looks better to me than having to transform with a regex all the code. In the plugin above I think you are also messing with source maps (something like magic string would be needed to rewrite the source maps there). Does this make sense?

@innocenzi
Copy link
Contributor

As it's served on http://localhost:3000/ but the local site is on https://webpack.livestorm.local/

That sounds basically like my use case. First URL is the development server URL, second is the local site one. Assets URLs need to have http://localhost:3000/ prepended instead of being just the paths. And http://localhost:3000/ is the URL we see on the console when the server is started.

In other words, to clear it up, instead of /path/to/asset.png we need http://localhost:3000/path/to/asset.png.

I'm not sure why the boolean option would not be enough, but as long as the proposed solution allows to have a full URL with an origin, it's good enough for me. Although the boolean option was appealing because it's simple, it is indeed nice to have more control, so allowing to pass a string seems fine.

One problem with the proposed approach is that some URL are not touched by Vite.

Do you mean that some URLs are transformed at build-time, but not because of Vite? I'm not sure to understand that point.

Also the term "public path" is confusing to me, I'm not sure what it refers to...

What I don't really understand is the fact that the production build outputs the right URLs, so they are all passed through Vite at some point, right? I don't know the internals very well but I really thought your previous suggestion would solve the issue.

@Akryum
Copy link
Contributor

Akryum commented Jul 27, 2021

One problem with the proposed approach is that some URL are not touched by Vite.

So far I've only needed to add the origin to things handled by Vite: imported files (we also import all the images in JS files), files referenced in CSS/PostCSS/Stylus...

@guradia
Copy link

guradia commented Aug 30, 2021

What I don't really understand is the fact that the production build outputs the right URLs, so they are all passed through Vite at some point, right? I don't know the internals very well but I really thought your previous suggestion would solve the issue.

Coming from the overall problem with a symfony project i like to add on this detail:
After a built the assets reside within a public directory of the symfony project and no confusion with ports can happen.

To elaborate further and picking up @Akryum 's last comment:
The whole problem only effects assets that are handled by vite (mostyle because they otherwhile are not in a public directory to begin with). But vite does handle the assets correctly in terms of pathes. Once you manually add the port to any not found assets the dev server has it ready at the exact path it produced.

To add an Example:

On a dev server starting up with:

Local: https://octopussy.dev.local:3000/dist/

a image (from a non-public directory ) i reference in CSS is available at:
https://octopussy.dev.local:3000/dist/assets/octopus-mean-teal.svg

But vite puts it in the CSS without :3000 (the whole Host-Part)
/dist/assets/octopus-mean-teal.svg

@innocenzi
Copy link
Contributor

To sum up the issue:

  • A page is loaded at http://custom-url.test
  • This page loads an asset, image.png
  • In development, this asset's path is stripped from its origin by Vite, resulting in http://custom-url.test/image.png instead of http://localhost:3000/image.png

This implementation solves the issue by allowing us to define an origin (server.publicPath), so the resulting generated asset URL will be the right one.

The build part was working as expected already, this implementation only changes the development one, so this is what we needed.

@patak-dev patak-dev added p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) and removed p2-nice-to-have Not breaking anything but nice to have (priority) labels Sep 21, 2021
Comment on lines +534 to +538
export default {
server: {
publicPath: 'http://127.0.0.1:8080/'
}
}
Copy link
Member

Choose a reason for hiding this comment

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

We try to use defineConfig everywhere in the docs, so folks know that this is a thing and prefer to use it

Suggested change
export default {
server: {
publicPath: 'http://127.0.0.1:8080/'
}
}
export default defineConfig({
server: {
publicPath: 'http://127.0.0.1:8080/'
}
})

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.

The option should be called server.origin instead of server.publicPath, other than that, we are good to merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
7 participants