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: build.modulePreload options #9938

Merged
merged 9 commits into from
Sep 24, 2022
Merged

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Aug 31, 2022

Fixes #9680
Fixes #3133
Fixes #5991
Fixes #5120 (it was closed as won't implement)
Closes #7766

Description

See the issues above for examples of requests to be able to control module preload. We already discussed in a team meeting that we should implement support for completely disabling module preload and also to support filtering.

DONE: Pushing a WIP, so we can discuss the API, @danielroe @antfu maybe there is a chance to include this in 3.1 (or we can later add it to a patch). It is WIP, because we should keep the .css chunks... even when preload is disabled as the mechanism to preload .js chunks and add the needed styles is shared (in the PR currently I passed both through modulePreload.resolveDependencies).
Question: Should we only allow modifying the .js chunks? Or should we also pass the .css assets to resolveDependencies and expect the user to avoid filtering them?

Edit: The new API will only receive an array of modules, CSS assets aren't included and are processed separatedly

docs

The polyfill can be disabled using modulePreload: { polyfill: false }. The previous polyfillModulePreload option has been deprecated.

The list of chunks to preload for each dynamic import is computed by Vite. By default, an absolute path including the base will be used when loading these dependencies. If the base is relative ('' or './'), import.meta.url is used at runtime to avoid absolute paths that depends on final the deployed base.

Edited after 9f1eaa5 (#9938)

There is experimental support for fine grained control over the dependencies list and their paths using the resolveDependencies function. It expects a function of type ResolveModulePreloadDependenciesFn

type ResolveModulePreloadDependenciesFn = (
  filename: string,
  deps: string[],
  context: {
    hostId: string,
    hostType: 'html' | 'js'
  }
) => string[]

The resolveDependencies function will be called for each dynamic import with a list of the chunks it depends on. A new dependencies array can be returned with these filtered or more dependencies injected, and their paths modified. The deps paths are relative to the build.outDir.

    modulePreload: {
        resolveDependencies: (url, deps, { importer }) => {
            return deps.filter(condition)
        }
    }

The paths can be further modified by experimental.renderBuiltUrl. See new config file in preload test case and updated docs.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@patak-dev
Copy link
Member Author

Pushing a WIP, so we can discuss the API, @danielroe @antfu maybe there is a chance to include this in 3.1 (or we can later add it to a patch). It is WIP, because we should keep the .css chunks... even when preload is disabled as the mechanism to preload .js chunks and add the needed styles is shared (in the PR currently I passed both through modulePreload.resolveDependencies). Question: Should we only allow modifying the .js chunks? Or should we also pass the .css assets to resolveDependencies and expect the user to avoid filtering them?

I think we should remove the .css assets from this new API. They have nothing to do with modulePreload except for the shared mechanism. But this means we will still need a way to resolve the CSS paths in the array (IIUC, Nuxt will also need to wrap these paths in a function call). An option is to have a new css.renderAsyncChunkPath(url, cssChunkPath, { importer }) called for each CSS chunk that needs to be loaded for a dynamic import. This only lets you affect the path but not filter or inject other .css as that doesn't make sense in this context.

Comment on lines 492 to 502
if (typeof dep === 'object') {
if (dep.runtime) {
return dep.runtime
}
if (dep.relative) {
return JSON.stringify(toRelative(dep.relative))
}
throw new Error(
`Invalid dependency object, no runtime or relative option specified`
)
}
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be a very advanced feature, especially the runtime part. I'm curious how Nuxt will be using this part of the API. Do they have their own runtime preload list?

IIUC this PR would also disable the optimization in #9491, or perhaps doesn't work only if resolveDependencies is used. But I don't think it's a big issue since most won't be using this feature directly.

The rest looks great though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nuxt is using runtime to use its own function to resolve paths, like globalThis.__resolvePath(...).

I commented in #9491, that we should wait a bit to avoid breaking the ecosystem but I think we can still do the optimization after this PR.

The PR changed quite a bit after 9f1eaa5 (#9938)

docs/config/build-options.md Outdated Show resolved Hide resolved
docs/config/build-options.md Outdated Show resolved Hide resolved
docs/config/build-options.md Outdated Show resolved Hide resolved
packages/vite/src/node/plugins/importAnalysisBuild.ts Outdated Show resolved Hide resolved
patak-dev and others added 4 commits September 3, 2022 18:18
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Tony Trinh <tony19@gmail.com>
Co-authored-by: Tony Trinh <tony19@gmail.com>
@patak-dev
Copy link
Member Author

After 9f1eaa5 (#9938), build.modulePreload.resolveDependencies will be called both for JS dynamic imports preload lists and also for HTML preload lists for chunks imported from entry HTML files.
And experimental.renderBuiltUrl will get called for the preload asset paths too, @danielroe this is what you were proposing at first and I thought it wasn't possible. So for Nuxt, you may end up only using renderBuiltUrl if you don't need to filter any of the preloads

@benmccann also interested in seeing if this PR is enough for SvelteKit module preload needs. The ssr manifest isn't modified by this PR, if you use that you have full control of the paths already. SvelteKit may need to replicate what we have done here for HTML, and for JS dynamic imports it should be now able to take advantage of experimental.renderBuiltUrl if needed.

@patak-dev patak-dev added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Sep 3, 2022
@patak-dev patak-dev added this to the 3.2 milestone Sep 3, 2022
@sapphi-red
Copy link
Member

sapphi-red commented Sep 7, 2022

The API spec looks great to me. (Though I've never created a meta-framework, so I don't have much confidence)

About the name of resolveDependencies, given that most users won't need to use it, maybe it's better to have advanced or something like that as a prefix?
But as experimental.renderBuiltUrl does not have any prefixes like that, it might be better to leave it as-is.

@patak-dev
Copy link
Member Author

Thanks for the review @sapphi-red. I would prefer to avoid the name advanced here, and in any case add an Advanced level in docs. This API in particular isn't as complex as renderBuiltUrl. I'm not married with the modulePreload.resolveDependencies name though, but I haven't found a better one.

@paulcgdev
Copy link

Which version is this experimental feature targeted for release? Hopefully, it's asap... 😄

@jpvincent
Copy link

jpvincent commented Sep 13, 2022

Hi
Nice to see this option appearing.
I dont have an opinion about the naming and all, but I would add that we need to also filter CSS preloads.

It will even allow to solve a bug I saw today : in 3.1 the preloads are made with a HTTP header, and the header is several Kb long … that's more than the nginx default value of 1Kb, leading to our pre-production refusing to serve the page.

EDIT : the bug has been created : sveltejs/kit#6790

@patak-dev
Copy link
Member Author

@jpvincent we also discussed with @danielroe yesterday about being able to modify the injected CSS styles for a chunk. But these aren't module preloads, the CSS chunks are in the same list just because we are piping them through the same function but the result is an injected stylesheet that is awaited. I think we need a new API for these, independent of this PR. I think a new feature should be created so we gather enough use cases to justify it.

patak-dev and others added 3 commits September 23, 2022 11:02
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
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.

Looks good!

@kleinfreund
Copy link

For those coming here after upgrading to Vite v4 and finding that suddenly their index.html file has a bunch of new <link rel="modulepreload" ...> entries, this can be turned off with:

{
  // ...
  build: {
    modulePreload: {
      resolveDependencies: () => [],
    },
  },
}

Most importantly, build.modulePreload set to false does not turn this off. Is this a bug? Happy to file a bug report if so.

This should be marked as a breaking change in the docs for migrating from v3 (https://vitejs.dev/guide/migration.html).

@MariuzM
Copy link

MariuzM commented Jan 31, 2023

Setting to false i get less bundle size

dist/assets/react-3610d2ce.js             158.04 kB │ gzip:  51.31 kB
dist/assets/index-f9ae9d49.js             465.23 kB │ gzip: 140.08 kB
dist/assets/react-3610d2ce.js             158.04 kB │ gzip:  51.31 kB
dist/assets/index-f5c7abff.js             464.52 kB │ gzip: 139.81 kB

And i dont get double file fetching in FF

@tkapitein
Copy link

tkapitein commented Feb 1, 2023

For those coming here after upgrading to Vite v4 and finding that suddenly their index.html file has a bunch of new <link rel="modulepreload" ...> entries, this can be turned off with:

{
  // ...
  build: {
    modulePreload: {
      resolveDependencies: () => [],
    },
  },
}

Most importantly, build.modulePreload set to false does not turn this off. Is this a bug? Happy to file a bug report if so.

This should be marked as a breaking change in the docs for migrating from v3 (https://vitejs.dev/guide/migration.html).

I was able to make sure the preloads where removed from my index.html file with this. However, we do use React.lazy quite extensively through our application and see that, with Vite 4, on initialization the app still loads a lot of files that should be lazy loaded when the application hits a certain route. This worked fine earlier with Vite 3.2.2.

@patak-dev
Copy link
Member Author

@kleinfreund would you create a new issue with a minimal reproduction for the issue you are experiencing? Thanks!

@kleinfreund
Copy link

@patak-dev I’ve filed a bug report for build.modulePreload: false not working: #11889

As for the other matter I brought up in #9938 (comment), do you think it’s appropriate to say this should be documented as a breaking change? Do you also want me to file an issue for that?

@patak-dev
Copy link
Member Author

Thanks for creating the issue, I don't know yet. I think it is a regression rather than a breaking change.

@patak-dev
Copy link
Member Author

We are starting to gather feedback about modulePreload.resolveDependencies to stabilize it in a future Vite version here:

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
Archived in project
10 participants