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(css): bundle css as part of optimizeDeps #14467

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thebanjomatic
Copy link
Contributor

@thebanjomatic thebanjomatic commented Sep 25, 2023

@bluwy This is the recreation of #14434 which was accidentally closed when the target branch was merged. I was relying on esbuild 19 which was in a separate PR.


Description

This feature is intended to support component libraries that ship tree-shakable modules that include css imports.

When building with Vite, if the js files within your dependencies contain css, then at build time, the css will get pulled into the app's css bundles and will be tree-shaken as part of the build (if sideEffects: false in the dependency and the module which imported the css is also dropped). For dev builds, treeshaking doesn't happen, and instead, all of the css will wind up getting imported. The current behavior with optimizeDeps is that the css imports are externalized and preserved in the resulting .js deps bundles. This can result in a lot of css request being made on page load to the dev server. While css imports typically are faster to process than js imports (where this problem was already solved by pre-bundling the js for dependencies), it still isn't ideal.

This feature extends the default preoptimized dependency bundling to support css imports from within the dependency's .js files. Esbuild already supports this feature, but it does so by bundling the css to a single .css bundle that sits alongside the js bundle, and it removes all imports to the css files from the resulting js bundle. The implied use-case is that consumers of a library built with esbuild would need to use a style tag, or import the css file directly.

Since we can't really change the semantics of how the library is consumed as a result of optimizeDeps, we need to inject a single import statement into the .js bundle if it has a corresponding cssBundle file in the esbuild metadata file. We are just appending this import statement to the bottom of the file so that the sourcemaps are still functional.

The end result is that the css for these dependencies will now be bundled into a single .css request for each dep / chunk that contains imports to a css file.

Additional context

I work on a component library that has around 350 vue files, 260 of which have <style> blocks.
When this library is built, the resulting artifacts are individual .js files for each of the .vue components,
and 1 or more .css files depending on how many <style> blocks there are. These .css files are then
imported with a static import along the lines of:

// file: "./dist/component1.js"
import './component1.css'

One advantage of doing it this way is that the library css will be treeshaken so only components that are actively imported and used will have their css included in the application's css bundle.

This approach seems to be pretty portable across different application build tools, but produces sub-
optimal results when using vite dev mode. While the requests still complete pretty quickly, the
connection does wind up stalled because of the number of requests being made and it slows the
initial page load and refreshes down a bit.

The following screenshot is all entirely css requests (and represents a fraction of the total requests triggered by the initial app start:
image

With this PR, there is a single 410 kB .css request made instead with a minimal amount of stalling at the start
of the request:
image


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 PR Title 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.

@stackblitz
Copy link

stackblitz bot commented Sep 25, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Comment on lines +26 to +28
const contents = await readFile(path, 'utf-8')
const cssBundle = basename(meta.cssBundle)
await writeFile(path, `${contents}\nimport "./${cssBundle}";\n`)
Copy link
Contributor Author

@thebanjomatic thebanjomatic Sep 25, 2023

Choose a reason for hiding this comment

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

Currently we use the default behavior of write: true for the esbuild plugins, so we need to read the files and rewrite them if they had a css bundle. We could also change to use write: false and then use this plugin to write all output files to disk if we'd like to avoid writing to these files in dist twice. I didn't necessarily want to change this as there might be other esbuild plugins people are using with vite that rely on write: true.

@thebanjomatic thebanjomatic force-pushed the feat/optimize-deps-css2 branch 8 times, most recently from 10effe1 to 7fb726c Compare January 12, 2024 16:05
@thebanjomatic
Copy link
Contributor Author

Hey @bluwy, I'm wondering what I can do to get this PR back onto the dev team's radar for review. When the PR was initially raised it was during the 5.0 beta and you were all pretty busy getting that out the door, but I'm still motivated to work to get this merged if you find the concept agreeable.

@thebanjomatic thebanjomatic force-pushed the feat/optimize-deps-css2 branch from 7fb726c to b1e6557 Compare January 24, 2024 20:44
This feature is intended to support component libraries that ship tree-
shakable modules that include css imports.

When building with Vite, if the js files within your dependencies,
contain css, then at build time, the css will get pulled into the app's
css bundles and will be tree-shaken as part of the build (if
sideEffects: false in the dependency and the module which imported the
css is also dropped). For dev builds, treeshaking doesn't happen, and
instead, all of the css will wind up getting imported. The current
behavior with optimizeDeps is that the css imports are externalized and
preserved in the resulting .js deps bundles. This can result in a lot of
css request being made on page load to the dev server. While css imports
are faster to process than js imports, it still isn't ideal.

This feature extends the default preoptimized dependency bundling to
support css imports from within the dependency's .js files. Esbuild
already supports this feature, but it does so by bundling the css to a
single .css bundle that sits alongside the js bundle, and it removes all
imports to the css files from the resulting js bundle. The implied use-
case is that consumers of a library built with esbuild would need to use
a style tag, or import the css file directly.

Since we can't really change the semantics of how the library is
consumed as a result of optimizeDeps, we need to inject a single import
statement into the .js bundle if it has a corresponding cssBundle file
in the esbuild metadata file. We are just appending this import
statement to the bottom of the file so that the sourcemaps are
still functional.

The end result is that the css for these dependencies will now be
bundled into a single .css request for each dep / chunk that contains
imports to a css file.
@thebanjomatic thebanjomatic force-pushed the feat/optimize-deps-css2 branch from b1e6557 to 9b0c216 Compare January 24, 2024 20:46
@thebanjomatic
Copy link
Contributor Author

@patak-dev / @bluwy Sorry to ping again, but it's been a few weeks. Is there any chance of this making it into the 5.1 release?

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.

2 participants