Skip to content

feat: better dynamic component css props #11792

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.

Sign up for GitHub

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

Merged
Rich-Harris merged 5 commits into from
Jun 6, 2024
Merged

feat: better dynamic component css props #11792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fb6d781
feat: better dynamic component css props
paoloricciuti May 26, 2024
32a17af
chore: fix tests
paoloricciuti May 26, 2024
36791e2
chore: remove anchor from `$.component`
paoloricciuti May 26, 2024
bb9898a
chore: better old test fixing
paoloricciuti May 27, 2024
01e8afb
changeset
Rich-Harris Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/smart-spiders-fetch.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"svelte": patch
---

feat: always create wrapper `<div>` for `<svelte:component>` with CSS custom properties
37 changes: 23 additions & 14 deletions packages/svelte/src/compiler/phases/3-transform/client/visitors/template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -898,6 +898,27 @@ function serialize_inline_component(node, component_name, context) {
);
}

if (node.type === 'SvelteComponent') {
const prev = fn;
fn = (node_id) => {
let component = b.call(
'$.component',
b.thunk(/** @type {import('estree').Expression} */ (context.visit(node.expression))),
b.arrow(
[b.id(component_name)],
b.block([
b.stmt(
context.state.options.dev
? b.call('$.validate_dynamic_component', b.thunk(prev(node_id)))
: prev(node_id)
)
])
)
);
return component;
};
}

if (Object.keys(custom_css_props).length > 0) {
const prev = fn;
fn = (node_id) =>
Expand Down Expand Up @@ -2925,7 +2946,6 @@ export const template_visitors = {
b.stmt(
b.call(
'$.component',
context.state.node,
// TODO use untrack here to not update when binding changes?
// Would align with Svelte 4 behavior, but it's arguably nicer/expected to update this
b.thunk(
Expand All @@ -2949,19 +2969,8 @@ export const template_visitors = {
context.state.template.push('<!>');

let component = serialize_inline_component(node, '$$component', context);
if (context.state.options.dev) {
component = b.stmt(b.call('$.validate_dynamic_component', b.thunk(b.block([component]))));
}
context.state.init.push(
b.stmt(
b.call(
'$.component',
context.state.node,
b.thunk(/** @type {import('estree').Expression} */ (context.visit(node.expression))),
b.arrow([b.id('$$component')], b.block([component]))
)
)
);

context.state.init.push(component);
},
Attribute(node, context) {
if (is_event_attribute(node)) {
Expand Down
3 changes: 1 addition & 2 deletions packages/svelte/src/internal/client/dom/blocks/svelte-component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@ import { block, branch, pause_effect } from '../../reactivity/effects.js';
/**
* @template P
* @template {(props: P) => void} C
* @param {Comment} anchor
* @param {() => C} get_component
* @param {(component: C) => import('#client').Dom | void} render_fn
* @returns {void}
*/
export function component(anchor, get_component, render_fn) {
export function component(get_component, render_fn) {
/** @type {C} */
let component;

Expand Down
2 changes: 1 addition & 1 deletion ...es/svelte/tests/runtime-browser/samples/svelte-component-css-custom-properties/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export default test({
component.componentName = 'Slider2';
assert_slider_2();
component.componentName = undefined;
assert.equal(window.document.querySelector('div'), null);
Copy link
Copy Markdown
Member

@dummdidumm dummdidumm May 27, 2024

Choose a reason for hiding this comment

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

Was confused by this deletion, and checked the current Svelte 4 REPL once more:

  • when this is falsy initially, there's a div
  • but if it's switching truhy from to falsy, then it will remove the wrapping div

--> feels like a bug in Svelte 4
--> what does this say about this change? Can we do this change after all, or not? I think having it falsy initially is very rare, so very few people would've run into this bug before. Always having the div might catch people's CSS selectors offguard.

Copy link
Copy Markdown
Member Author

@paoloricciuti paoloricciuti May 27, 2024

Choose a reason for hiding this comment

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

I actually changed this to check that the div doesn't have a firstElementChild. But your reasoning still applies. That said i think the situations where this can trip people are the same where the component is actually there (more or less) which might be a bit more now that :has it's a bit more standard but shouldn't be that much because it's just if you are writing css based on the fact that a certain element follow another.

Copy link
Copy Markdown
Member Author

@paoloricciuti paoloricciuti May 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could it be worth passing the component to css_props to allow css_props to check for is thrutyness? And by that i mean literally the component that would be rendered...so for a normal component it would just be the component itself, for SvelteComponent the argument of this

Copy link
Copy Markdown
Member

@Rich-Harris Rich-Harris Jun 4, 2024

Choose a reason for hiding this comment

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

I'm inclined to ignore the falsy <svelte:component> edge case. As noted, it's already buggy (and to my knowledge no-one has encountered this bug in the wild), and even if it wasn't, the cases where your styles are affected by the presence of an unexpected <div style="display: contents"> with no children are vanishingly rare and easily worked around.

In other words I think it's fine to always have the wrapping div

assert.equal(window.document.querySelector('div')?.firstElementChild, null);
component.componentName = 'Slider1';
assert_slider_1();
}
Expand Down
2 changes: 1 addition & 1 deletion ...s/svelte/tests/runtime-browser/samples/svelte-component-css-custom-properties2/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export default test({
component.componentName = 'Slider2';
assert_slider_2();
component.componentName = undefined;
assert.equal(window.document.querySelector('div'), null);
assert.equal(window.document.querySelector('div')?.firstElementChild, null);
component.componentName = 'Slider1';
assert_slider_1();
}
Expand Down
3 changes: 1 addition & 2 deletions packages/svelte/tests/runtime-browser/samples/svelte-self-css-custom-properties2/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ export default test({
const nest_track_color1 = target.querySelector('#nest-component1 span');
const nest_rail_color2 = target.querySelector('#nest-component2 p');
const nest_track_color2 = target.querySelector('#nest-component2 span');

assert_ok(rail_color1);
assert_ok(track_color1);
assert_ok(rail_color2);
Expand Down Expand Up @@ -96,7 +95,7 @@ export default test({
component.componentName = 'Slider2';
assert_slider_2();
component.componentName = undefined;
assert.equal(window.document.querySelector('div'), null);
assert.equal(window.document.querySelector('div')?.firstElementChild, null);
component.componentName = 'Slider1';
assert_slider_1();
}
Expand Down