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: add load function to handle hook to control css loading #12677

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

Conversation

sviripa
Copy link

@sviripa sviripa commented Sep 15, 2024

This PR introduces an additional handle hook function, similar to preload. The purpose of this function is to control which CSS assets should be loaded for a specific page or route.

This functionality is necessary for dynamic component use case implemented using a combination of +page.server.js and +page.js. In the current version of SvelteKit, the CSS assets of all dynamically imported components are loaded on the page, regardless of whether or not they are actually used.

It is possible to remove unwanted css by redacting the html output using transformPageChunk but this feels more like a hack. It also does not work with inlineStyleThreshold configuration property.

The new function will allow SvelteKit users who use dynamic component imports to control which CSS assets should be loaded.

I'll address the items in the list below once I receive initial feedback on the solution. Thanks 🙏🏻

TODO:

  • Naming
  • Test
  • Docs
  • Changeset

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https:/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

P.S.

Huge thank you to everyone involved in Svelte and SvelteKit projects ❤️

Copy link

changeset-bot bot commented Sep 15, 2024

⚠️ No Changeset found

Latest commit: 9230536

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@sviripa sviripa changed the title feat: add load function to handle hook to control css loading feat: add load function to handle hook to control css loading Sep 15, 2024
@eltigerchino
Copy link
Member

Hi, thanks for the PR! I'm curious about how this would work. Could you provide an example of the usage of this new hook?

@dummdidumm
Copy link
Member

I'm not 100% sure if this use case warrants a new feature, and whether we should investigate loading the correct CSS instead (i.e. do the right thing automatically). This function would require very careful consideration of which CSS strings to load on which not, which feels only a little less hack than using transformPageChunk

@sviripa
Copy link
Author

sviripa commented Oct 7, 2024

@dummdidumm I agree that the proposed solution is only slightly less of a hack than using transformPageChunk; its only real advantage is that it is compatible with the inlineStylesThreshold config, whereas transformPageChunk is not.

I think it would be amazing if SvelteKit could automatically load the correct CSS for dynamically imported components. I was initially hesitant to suggest this because I wasn’t sure if it was possible or if it would be something SvelteKit team would accept.

I’m not 100% sure how to implement it, but I think I can figure it out if pointed in the right direction. Do you have any suggestions on how this could be implemented or which parts of SvelteKit’s codebase could be relevant?

@sviripa
Copy link
Author

sviripa commented Oct 7, 2024

@eltigerchino I made an example repo to showcase the issue.

Here's a link to the example repo and here it is deployed. On Foo page it renders two components so it is ok to load css bundles of all dynamically imported components. On Bar and Baz only one of the components is rendered but both css bundles are loaded.

In this example the new hook will be used to control the stylesheets loaded on the page based on the components property set in server load function.

@sviripa
Copy link
Author

sviripa commented Oct 7, 2024

@dummdidumm I’ve spent some time investigating this, and I believe it might be possible for SvelteKit to automatically filter out irrelevant CSS from dynamically imported components. However, my knowledge of the codebase is limited, so I would really appreciate it if you could confirm whether this makes sense and whether there are better ways to solve this issue.

The way I imagine this could work is if the framework provided developers with a mechanism to "notify" it of dynamic imports at runtime. The simplest implementation could look like the following:

export const load: PageLoad = async ({ dynamicImport }) => {
	const myDynamicComponent = await import(`../../components/${name}.svelte`);
	dynamicImport(`../../components/${name}.svelte`);
	
	return {
		myDynamicComponent
	}
}

When dynamicImport is called, it would push the import path to an request event scoped set. This set could then be used to filter out irrelevant CSS when rendering the page response.

The ergonomics of the code above are questionable at best. One potential way to improve it would be by creating a custom import function, like the following:

export const load: PageLoad = async ({ dynamicImport }) => {
	const myDynamicComponent = await dynamicImport(`../../components/${name}.svelte`);
	
	return {
		myDynamicComponent
	}
}

However, to actually make this work it would require a custom vite plugin that would be responsible for transforming the code above to the code from the first snippet. Transforming back to an import statement is necessary for vite-plugin-dynamic-import to work.

dynamic function described above would allow SvelteKit to track which entries/components were imported dynamically at runtime. The remaining task is to determine which CSS bundles are associated with those dynamic imports.

Currently, when server nodes are being built, the CSS bundles of all imported components (both static and dynamic) are added to the same stylesheets array.

CSS bundles are collected from the vite's client manifest using find_deps function call with add_dynamic_css set to true. Relevant code lines:

Instead nodes could have stylesheets array and dynamic_stylesheets map. dynamic_stylesheets key could be a node's dynamic import and a value will be an array of css bundles related to that import.

// Lists css assets of statically imported components
export const stylesheets = [];

export const dynamic_stylesheets = {
  // key is a dynamic import and value is a list of css assets associated with it.
  'src/components/DynamicComponent.svelte': [
    './src/components/DynamicComponent.css', 
    './src/components/DynamicComponentChild.css'
  ]
}

Please let me know if this makes sense or of additional info is needed. I would be happy to implement any or all of the above if necessary.

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

Successfully merging this pull request may close these issues.

3 participants