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

feat!: relative base #7644

Merged
merged 39 commits into from
May 18, 2022
Merged

feat!: relative base #7644

merged 39 commits into from
May 18, 2022

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Apr 7, 2022

fix #2009
fix #5270

Description

Makes built apps portable when using relative base (base: './' or base: '').

When using a relative base, there shouldn't be any absolute URL. Including paths to public files and paths to assets from HTML files.

Related discussions:

Why we need relative base support, from @Rich-Harris's comment:

  • the basepath is configured by an environment variable at runtime
  • the site is deployed with IPFS
  • you're building a Chrome extension
  • the site is being viewed on something like the Internet Archive

Update to modern browser targets

Edit: We discussed this in a team meeting and we have agreed to update the browser targets as described here in v3

We need support for import.meta.url to be able to find the assets URL in some cases (for the preload URLs for example).

import.meta is at 91.22%, and we are currently require dynamic import support so we are at 91.47%. Given that import.meta.url will enable us to implement features like this PR, and the numbers will keep getting better, I think we could change our default modern target in Vite v3 and push an extra 0.25% of users to legacy mode. See Discussion with @bluwy and @sapphi-red in Discord here

The new esbuild resolved target upgrades 'es2019' to 'es2020' and downgrades safari from 13.1 to 13 so we keep transpiling nullish coallesing (still at ~89%)

resolved.target = [
  'es2020', // support import.meta.url
  'edge88',
  'firefox78',
  'chrome87',
  'safari13' // transpile nullish coallesing
]

@vitejs/plugin-legacy is updated to check import.meta support (on top of dynamic import support):

const detectModernBrowserCode = `try{import(new URL(import.meta.url).href).catch(()=>1);}catch(e){}window.${detectModernBrowserVarName}=true;`

Asset URLs replacements in JS files

Edit: after a discussion with @bluwy, he proposed we keep using quotes and JSON.stringify and interpolate using " + ... + ".

Alternative interpolation strategy that is no longer used in the PR

For assets paths imported from a JS file, or for CSS inlined with asset URLs, we need to use import.meta.url to obtain a runtime absolute path because these strings can be re-exported and used in other files, or used directly in the DOM.

Instead of plain strings

return `export default ${JSON.stringify(css)}`

We could use template literals in these cases

// Return a template string so we can use interpolation when replacing __VITE_ASSET__
return `export default \`${css}\``

Allowing us to inject runtime interpolation with new URL(...,import.meta.url)

const replacement = isRelativeBase(config.base)
  ? `\${new URL(${JSON.stringify(outputFilepath)},import.meta.url)}`
  : outputFilepath
s.overwrite(match.index, match.index + full.length, replacement, {
  contentOnly: true
})

Note

During the Vite 2.0.0 beta, 00bc446 added relative base support, fixing #1669. Afterward, tests for relative base weren't included in Vite's CI. So the code evolved and we lost support for this feature.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev patak-dev requested a review from bluwy April 7, 2022 23:12
@patak-dev patak-dev added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Apr 7, 2022
@patak-dev
Copy link
Member Author

Tests are passing, except for Windows. We are probably missing a path normalization.

More testing is still needed, I changed some playgrounds to use relative base, and tested by hand in some others (there is a new build:portable script in the assets playground for example). It isn't easy to test a playground with both bases without doubling the time and making the expectation more complex. So maybe some playgrounds using relative base, a new portable playground for some specific checks, and adding the build:portable, preview:portable to most playgrounds for manual debugging could be enough.

@patak-dev patak-dev added this to the 3.0 milestone Apr 7, 2022
@sapphi-red
Copy link
Member

sapphi-red commented Apr 8, 2022

Looks like tests are failing because this line is missing normalizePath. (I have not checked all the code but at least it worked for playground/vue and playground/worker)

const file = path.resolve(path.dirname(id), rawUrl.slice(1, -1))

In addition, I think this PR will fix #5270 and #7279 since import.meta won't be transpiled.

@patak-dev
Copy link
Member Author

I don't know how disruptive it could be for the ecosystem, but relative paths seem like a better default to me than base: '/'. I think users could even open a built app from the filesystem without needing a local preview server 🤔
Should we analyze changing the default base in Vite 3.0? For sure we would need a major, we would need to wait if not another year.

Note: Right now, during dev even if the user sets base: './', Vite will cheat and use '/' that makes some handling easier in the dev server. I think this is fine, I'm thinking if we could still try to use relative base during dev as much as possible so Dev and Build are closer but I don't know if the extra complexity is justified.

@haoqunjiang
Copy link
Member

Posting my reply in the Discord channel here for a more public discussion:

The relative base path doesn't work well for code-splitting. That's why webpack introduced publicPath: 'auto' even though ./ already works. webpack/webpack#11127 (./ is relative to the page URL, auto is relative to the script URL, i.e. import.meta.url, which may be a CDN URL)

Note that even if we follow the webpack approach, the auto public path has some edge cases in legacy browsers that cannot be fully resolved:
Webpack recommends falling back to currentScript in legacy bundles, but it's not equivalent to import.meta.url in functionality. It will fail in callbacks or event handlers.
https://developer.mozilla.org/en-US/docs/Web/API/Document/currentScript

It's important to note that this will not reference the <script> element if the code in the script is being called as a callback or event handler; it will only reference the element while it's initially being processed.

@patak-dev
Copy link
Member Author

I think what this PR is doing for './' is similar to Webpack's publicPath: 'auto', as it is using import.meta.url as you describe.

I like having a base: 'auto', it is more clear than './' or '' because that isn't the base used all the time. For simplicity, we could make all of them work in the same way, since './' is the same as 'auto' in the cases where it works. Maybe we could add a warning when the user uses './' or '' deprecating them and pointing to the new 'auto' instead.

packages/vite/src/node/build.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/worker.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/importAnalysisBuild.ts Outdated Show resolved Hide resolved
@patak-dev patak-dev mentioned this pull request Apr 13, 2022
9 tasks
patak-dev and others added 2 commits April 13, 2022 21:30
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@patak-dev
Copy link
Member Author

@benmccann would you be able to review this PR and check if most use cases discussed in sveltejs/kit#595 (or involve others from the Svelte community that may be interested in helping push this change forward?)

@wighawag it would be good if you could check this PR, and see if it would help you with https://github.com/wighawag/sveltejs-adapter-ipfs

For context, this PR may be later complemented with an option to define the URL for the assets path dynamically (and public files path, but I think SvelteKit isn't using the public folder), #3522 (comment) (still keeping relative as many of the URLs as possible)

@benmccann
Copy link
Collaborator

@benmccann would you be able to review this PR and check if most use cases discussed in sveltejs/kit#595 (or involve others from the Svelte community that may be interested in helping push this change forward?)

@Rich-Harris might be the best person to take a look at this one. I see @dominikg and @bluwy have both taken a look at this PR as well though

packages/vite/src/node/plugins/worker.ts Outdated Show resolved Hide resolved
packages/vite/src/node/utils.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/asset.ts Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/asset.ts Outdated Show resolved Hide resolved
patak-dev and others added 2 commits April 29, 2022 20:37
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@Rich-Harris
Copy link
Contributor

Confirmed that this now works for a SvelteKit test case, the only change needed being to change base in our internal config to ./. Wonderful work, thank you so much @patak-dev! Can't wait to use this 😍

benmccann
benmccann previously approved these changes May 17, 2022
@patak-dev patak-dev dismissed stale reviews from benmccann and antfu via f785bed May 17, 2022 21:26
@patak-dev
Copy link
Member Author

We already discussed with the team and approved changing the modern browser targets increasing the baseline to browsers supporting import.meta.url (that would be useful moving forward not only for this PR). The implementation got a few reviews in the past few days (see reviews invalidated by added test cases and minor fixes in the last commits). And we now have success stories from SvelteKit and partial testing also from Nuxt (@danielroe was also testing it yesterday).

Let's merge the PR so we can start building on top of it. There are changes I think would be good to make but it will be easier to discuss them in separate PRs. It is also better if we can later release a new v3 alpha patch with it so we start getting more testing from the ecosystem.
Thanks to the folks that tested along the way, please create issues from now on so we can stabilize things for v3. @wighawag I'm especially interested in your use case as it will be a big stress test for these changes.

@patak-dev patak-dev changed the title feat: relative base feat!: relative base May 18, 2022
@patak-dev patak-dev merged commit 09648c2 into main May 18, 2022
@patak-dev patak-dev deleted the feat/relative-base branch May 18, 2022 05:07
@patak-dev
Copy link
Member Author

Released as part of vite@3.0.0-alpha.1

@evilgeniuscreative
Copy link

I still find this very difficult to actually make happen, and the documentation is not as clear on how to do it and specifically what it does as I think Vite thinks it is.

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