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

__vitePreload dependencies could be relative to import.meta.url #2009

Closed
Rich-Harris opened this issue Feb 13, 2021 · 13 comments · Fixed by #7644
Closed

__vitePreload dependencies could be relative to import.meta.url #2009

Rich-Harris opened this issue Feb 13, 2021 · 13 comments · Fixed by #7644
Labels
enhancement New feature or request p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Milestone

Comments

@Rich-Harris
Copy link
Contributor

Describe the bug

__vitePreload includes assumptions about where assets are being served from. In a normal Vite project those assumptions hold true, but in more bespoke situations they may not.

Additionally, in the case where CSS dependencies fail to load (whether due to a network failure, or because of the situation described above) the promise returned from __vitePreload will never resolve.

Reproduction

https://github.com/Rich-Harris/vite-preload-repro

Given sources like this...

// src/index.js
import('./foo.js').then((foo) => {
  foo.hello();
});
// src/foo.js
import './foo.css';

export function hello() {
  document.body.innerHTML = `<h1>Hello!</h1>`;
}
// src/foo.css
h1 {
  color: red;
}

...and a config like this...

const config = {
  build: {
    cssCodeSplit: true,
    lib: {
      entry: resolve('src/index.js'),
      name: 'app', // this doesn't seem to do anything?
      formats: ['es']
    },
    outDir: 'dist',
    minify: false,
    rollupOptions: {
      output: {
        entryFileNames: '[name]-[hash].js',
        chunkFileNames: '[name]-[hash].js',
        assetFileNames: '[name]-[hash][extname]'
      }
    }
  }
};

...the resulting dist/index-ed8ec7ff.js file contains this line:

__vitePreload(() => import("./foo-5368ca0f.js"), true ? ["/foo-5368ca0f.js","/foo-0a0d0fb0.css"] : void 0).then((foo) => {

In the case where assets are served from somewhere other than / (for example, on an external CDN), the /foo-5368ca0f.js and /foo-0a0d0fb0.css paths are meaningless. If they were relative paths instead, the implementation of __vitePreload could add the following...

  return Promise.all(deps.map((dep) => {
+   dep = new URL(dep, import.meta.url).href;
    if (dep in seen)
      return;

...and the resulting files would be more portable.

Meanwhile, to prevent the promise from hanging:

    if (isCss) {
-     return new Promise((res) => {
-       link.addEventListener("load", res);
+     return new Promise((res, rej) => {
+       link.addEventListener("load", res);
+       link.addEventListener("error", rej);
      });
    }

System Info

  • vite version: 2.0.0-beta.69
  • Operating System: Mac OS
  • Node version: 12.18.3
  • Package manager (npm/yarn/pnpm) and version: npm 6.14.6
@yyx990803
Copy link
Member

It does take the base option into account - or do you intend to deploy it at unknown base locations?

@yyx990803
Copy link
Member

Oh it looks like you are using Vite to build a lib with custom output filenames... yeah the current preload does assume an app with a known base location and can be improved.

@Rich-Harris
Copy link
Contributor Author

Actually, in my case base should suffice — thanks, I looked for an option like that but didn't realise that was what I wanted. It's definitely possible to imagine a situation where the base path is unknown at build time, but it's perhaps less likely.

Rejecting the promise if the link errors might be worth doing independently of that anyway — would a PR be useful?

@jonaskuske
Copy link
Contributor

jonaskuske commented Feb 13, 2021

definitely possible to imagine a situation where the base path is unknown at build time

Came across that situation yesterday! (wanted the app to work from /wp-content/themes/{someDirectory} and ideally also from /)

Setting base to "" (relative) and then using a <base> tag works! ¹


¹ unless you depend on the dynamic import polyfill because this upstream PR to resolve from document.baseURI instead of location.href was never merged

@yyx990803
Copy link
Member

Rejecting the promise if the link errors might be worth doing independently of that anyway — would a PR be useful?

Absolutely!

@olemarius
Copy link

I have the same issue. We build locally where it will find the files using relative path, but in production the files are located on a cdn while website on a different webserver, so we're getting a bunch of 404's in the network traffic, before the same files are loaded using the base url.

So the current behavior still loads the files, but the expected behavior would be to point the preload to the cdn, respecting the base url.

@kayakyakr
Copy link

Another use case that this breaks is when trying to launch a sveltekit app from a chrome extension. This is a big deal in a content script, especially, where it's trying to preload those links via the embedded page's url.

I'm actually skipping the generated page to run the start script directly, but the module preload still throws a bunch of errors, and all of the css fails to load.

I'm still working out exactly what needs to happen and if there's a workaround, but this is pretty much putting the kibosh on my dreams of running sveltekit from an extension.

@kayakyakr
Copy link

I found a workaround presuming I have a stable chrome extension ID: I can provide chrome-extension://1234 as the asset base. Unfortunately, vite works against me here because of this method:

export const externalRE = /^(https?:)?\/\//
export const isExternalUrl = (url: string): boolean => externalRE.test(url)

where chrome-extension is not a valid external URL. Would appreciate a fix for that aspect, at least.

@gaocheng001
Copy link

gaocheng001 commented May 7, 2021

它确实考虑了基本选项-还是打算将其部署在未知的基本位置?

配置选项build.lib中formats: ['es'],作为懒加载控件使用,因为控件内所有懒加载js应该相对于js位置,但使用时404

__vitePreload让我很头疼,现在不知道该怎么处理

@hirasso
Copy link

hirasso commented Nov 1, 2021

Hi there! I Thank you for this amazing project.

After a lot of fiddling around with the base option, I found, that in my setup, it would also make sense to use import.meta.url instead of the current browser URL, if it was set to ./. Intuitively, I would expect that links to assets (js import or css url()) would resolve relative to their respective referencing file, not relative to whatever my website's current URL is. Here's my project tree (It's a classic PHP website, the setup is loosely following the approach described here):

.
├── index.php
└── subfolder
   ├── assets # my equivalent of 'dist' folder
   ├── assets-src # my equivalent of default 'src' folder
   ├── composer.json
   ├── composer.lock
   ├── lib
   ├── node_modules
   ├── package-lock.json
   ├── package.json
   └── vite.config.js

My vite.config.js looks like this:

import { resolve } from 'path';

import { defineConfig } from 'vite'
import liveReload from 'vite-plugin-live-reload'

export default defineConfig(({ command, mode }) => {
  return {
    base: './', // resolves correctly for css assets when being built. doesn't resolve in either scenarios for js import statements
    build: {
      target: 'es2015',
      outDir: 'assets',
      rollupOptions: {
        input: {
          frontend: 'assets-src/frontend.js',
          admin: 'assets-src/admin.js'
        },
        output: {
          entryFileNames: `[name].js`,
          chunkFileNames: `[name].js`,
          assetFileNames: `[name].[ext]`
        }
      },
    },
    server: {
      host: 'vite-with-backend.test',
      port: 3000,
      strictPort: true
    },
})

I was able to solve this by using an absolute path as base in different contexts, like this:

const basePath = __dirname.split('/wwwroot')[1]; // wwwroot is the very root folder
base: command === 'serve' ? basePath : `${basePath}/assets/`,

...but I often have the scenario that my local development path is different to the path in production (e.g. when I install a website on the remote server inside a sub-directory, but my local site is running in the root).

My suggested solution would be to detect if base is set to ./, and if so, replace it like this:

let base = "./";
if( base === "./" ) base = new URL(import.meta.url).pathname.replace(/\/[^\/]+$/, '/');

@eigan
Copy link

eigan commented Feb 15, 2022

Had the same problem as @olemarius. Solved it with https://github.com/jy0529/vite-plugin-dynamic-publicpath

@benmccann
Copy link
Collaborator

Actually, in my case base should suffice

You can set the base path now, but it's turned out not to be sufficient for the SvelteKit use case. It's still be nice to remove that and make it relative. E.g. for deploying to IPFS and other targets as described in sveltejs/kit#595

@benmccann benmccann added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Mar 1, 2022
@pavelloz
Copy link

pavelloz commented Mar 2, 2022

@eigan Thank you. Ive been looking for this plugin for quite some time.

@patak-dev patak-dev added this to the 3.0 milestone Apr 4, 2022
This was referenced Apr 6, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging a pull request may close this issue.