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: experimental.renderBuiltUrl (revised build base options) #8762

Merged
merged 10 commits into from Jun 27, 2022

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Jun 24, 2022

Description

Alternative design to

Nuxt is planning to release an RC using the new experimental support for advanced base options, as it greatly improves the robustness of path handling for them (that before were using a post regex based approach)

We discussed with @danielroe and revised the API now that we have more experience with real usage.

API Simplification

Edited with the lastest changes in the PR

Inversion of control to simplify the API, making it more flexible. The option can now be only of type:

export type RenderBuiltAssetUrl = (
  filename: string,
  {
    hostId: string,
    hostType: 'js' | 'css' | 'html'
    type: { type: 'asset' | 'public' 
    ssr: boolean 
  }
) => string | { relative?: boolean, runtime?: string } | undefined

Example:

experimental: {
  renderBuiltAssetUrl: (filename, { hostType }) => {
    if (hostType === 'js') {
      return { runtime: `window.__toCdnUrl(${JSON.stringify(filename)})` }
    } else {
      return { relative: true }
    }
  }
}
  • We no longer need separate config options for { public, assets } as in feat: experimental.buildAdvancedBaseOptions #8450, as the asset type (hashed assets: 'asset', public files: 'public') is passed to the function. The user can decide to treat both in the same way or do something specific for one of them.
  • We no longer need a relative option. The general relative option is set with base: './' as before, and the user can decide to change the default by returning { relative: boolean } for one of the assets.
  • We no longer need a url option. The user should return a proper path from the renderBuiltAssetUrl function.
  • We no longer need a runtime option. The user can return { runtime: code } if the importer is a JS file. An error will be thrown if this is used in a different context

Other changes

  • Both hashed assets and public files paths are given to the function as a path starting from the generated dist and without an initial / (prev public files started with /)
  • Both of them also are proper paths without the prev "..." wrapping. @danielroe I know we discussed that it would be better to keep the wrapping, but we needed this change to be able to return a path from the function as string instead of a wrapped path. This function is not only for runtime now.
  • The PR now doesn't allow the use of runtime modification for module preloads. These are not really needed as relative base works well here. We already discussed with the team adding a modulePreload config entry with filtering and transform, so we are going to be able to do this in the future through it if needed.

New possibilities

The previous runtime option was only called for JS importers. The new renderBuiltAssetUrl is called for assets referenced in JS, CSS, and HTML. This may allow us in the future to create dynamic paths in CSS using CSS Variables (still not possible, but probably it will with Houdini).

The new function also takes an ssr flag as there are times when the paths should be different. For example, relative is ignored in SSR. @danielroe I think the flag is useful here to avoid a conditional config (and especially thinking about converting this to a hook), but we could end up removing it in the final version depending on usage.

The name is inspired by other renderYYY hooks in rollup. I think we could make this option a hook in the future and the design is thought to enable us to do so. Keeping it as an experimental config option allows us to more easily change the design and avoid breaking compat so IMO is a good idea to wait until we are sure this is going to be the final version to convert it to a hook.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@netlify
Copy link

netlify bot commented Jun 24, 2022

Deploy Preview for vite-docs-main ready!

Name Link
🔨 Latest commit 1f90085
🔍 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62b9e0fb3df4c20008fba8cf
😎 Deploy Preview https://deploy-preview-8762--vite-docs-main.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@patak-dev
Copy link
Member Author

patak-dev commented Jun 24, 2022

@danielroe about your use case

{ runtime: `__nuxt_toAssetPath(${JSON.stringify(url)})` }

with __nuxt_toAssetPath a local function imported by the chunk. I think we could still try to use renderHook in a plugin to detect __nuxt_toAssetPath and then import it if needed. Or we could explore some kind of { inject: { name: '__nuxt_toAssetPath', from: 'globals' } }. But we need to check how to specify the path as we are already in renderChunk here.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Not following all the base change lately, but I like this API more too

packages/plugin-legacy/src/index.ts Outdated Show resolved Hide resolved
packages/vite/src/node/build.ts Outdated Show resolved Hide resolved
bluwy
bluwy previously approved these changes Jun 24, 2022
@patak-dev
Copy link
Member Author

Renamed to renderBuiltUrl(), so there is no confusion that it both applies to public files and hashed assets. It also aligns with the proposed resolveBuiltUrl if we ever merge that (this hook would be called before converting to VITE_ASSET, but at this point we don't know the chunk type or name to do it relative):

The proposed config option in this PR (maybe future hook) would be an extended version of https://rollupjs.org/guide/en/#resolvefileurl, but working also with public files (we can't use this rollup hook, see here:

// rollup supports `import.meta.ROLLUP_FILE_URL_*`, but it generates code

About the API, resolveFileUrl takes directly an object. We could align more but it isn't easy:

  • It uses chunkId, and we want to also pass HTML files as a importer. Maybe importer isn't the best name. hostId could be better?
  • It passes relativePath. We could also pass include. The PR implements support for { relative: true } that I think is easier to use though. But maybe not a bad idea to have it.

@patak-dev
Copy link
Member Author

One more thing about the API, I think we should pass the hostType, having to use the importer isn't that good with all the suffix dance, still good to include it though. I'll modify the PR.

@danielroe I tried to do a plugin that uses this config together with generateBundle. It feels a bit brittle to need to relay on regexes and add an import, so I wouldn't recommend the approach.

Another option is to have a plugin that on transform injects the import as you wanted. You can use moduleSideEffects: 'no-treeshake' when loading the module to tell rollup to avoid treeshaking it even if its contents are unused. One way would be to rely on regexes but again doesn't look like a robust solution.
Another option is to always include this import and let rollup deal with handling it. See commit: chore: example of helper from virtual module

@patak-dev patak-dev changed the title feat: experimental.renderBuiltAssetUrl (revised build base options) feat: experimental.renderBuiltUrl (revised build base options) Jun 27, 2022
danielroe
danielroe previously approved these changes Jun 27, 2022
@patak-dev
Copy link
Member Author

patak-dev commented Jun 27, 2022

Merging soon so Nuxt can continue to test this API in the next few days. Discussed with @danielroe offline that this seems a lot more flexible than the buildAdvancedBaseOptions API.

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@patak-dev patak-dev merged commit 895a7d6 into main Jun 27, 2022
@patak-dev patak-dev deleted the refactor/advanced-base-options-revision branch June 27, 2022 18:28
Comment on lines +48 to +64
{
name: 'virtual-module',
resolveId(id) {
if (id === virtualFile) {
return virtualId
} else if (id === nestedVirtualFile) {
return nestedVirtualId
}
},
load(id) {
if (id === virtualId) {
return `export { msg } from "@nested-virtual-file";`
} else if (id === nestedVirtualId) {
return `export const msg = "[success] from conventional virtual file"`
}
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

@patak-dev this plugin it seems is already defined above 👀

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha, good catch! Would you like to do a PR to remove the second one from the config?

Copy link
Contributor

Choose a reason for hiding this comment

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

will do

#8833

@ropez
Copy link

ropez commented Jul 11, 2022

renderBuildUrl doesn't seem to be called for JS bundles created by async components and routes.

The old "advanced base options" supported this, was it removed in this version??

@patak-dev
Copy link
Member Author

@ropez it should be called for the import path of the bundle, but not for preloads. We've decided to remove preload as they always work when you define relative base (base: '') because they are assets to assets paths. We also plan to add a new API to disable, filter and transpile preload paths later.

@francislavoie
Copy link

francislavoie commented Jul 13, 2022

I'm having trouble with the TS types for renderBuiltUrl. They don't seem to match the implementation. The documentation https://vitejs.dev/guide/build.html#advanced-base-options also seems incorrect (bad syntax for the 2nd param).

Edit: Got it, the docs should read like this:

experimental: {
  renderBuiltUrl: (filename, { hostType }) => {
    if (hostType === 'js') {
      return { runtime: `window.__toCdnUrl(${JSON.stringify(filename)})` }
    } else {
      return { relative: true }
    }
  }
}

i.e. remove the types on filename and hostType

@wilcoxmd
Copy link

wilcoxmd commented Apr 27, 2023

👋 Hi there! We're looking at using this feature and I was curious if there are any currently planned updates to the API. Fully aware that it's still under the experimental label in Vite v4, so it's probably not possible to promise no changes, and that's OK. I'm just curious if there is any general sense of how final this API is now that it's been out in the world for ~8 months, or any estimate on when it will be promoted.

@patak-dev
Copy link
Member Author

@wilcoxmd Nuxt and others are using this API already. You can safely use it. If we change something, it will be in a Major now. We'll review if in Vite 5 we can remove from experimental these features that have already been in Vite for a while.

@wilcoxmd
Copy link

Amazing. Thanks for the quick update @patak-dev! Looking forward to trying this out.

@patak-dev
Copy link
Member Author

We're thinking of stabilizing this feature in Vite 5. If you have feedback, please comment in this discussion:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants