Skip to content

fix: avoid shadowing a variable in dynamic components #16185

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
merged 2 commits into from
Jun 17, 2025
Merged

fix: avoid shadowing a variable in dynamic components #16185

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d0794a
fix: avoid shadowing a variable in dynamic components
7nik Jun 16, 2025
2fa8c6d
split component name and intermediate name
7nik Jun 17, 2025
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/short-mails-know.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

fix: avoid shadowing a variable in dynamic components
7 changes: 4 additions & 3 deletions packages/svelte/src/compiler/phases/3-transform/client/visitors/Component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,10 @@ import { build_component } from './shared/component.js';
export function Component(node, context) {
const component = build_component(
node,
// if it's not dynamic we will just use the node name, if it is dynamic we will use the node name
// only if it's a valid identifier, otherwise we will use a default name
!node.metadata.dynamic || regex_is_valid_identifier.test(node.name) ? node.name : '$$component',
// avoid shadowing the component variable by a variable used in $.component
node.metadata.dynamic
Copy link
Contributor

@elliott-with-the-longest-name-on-github elliott-with-the-longest-name-on-github Jun 16, 2025

Choose a reason for hiding this comment

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

Don't we need to keep the regex_is_valid_identifier test here for the node.name branch?

Also IMO we should cut this out and move it into its own variable -- it's getting hard to read

Copy link
Contributor

@elliott-with-the-longest-name-on-github elliott-with-the-longest-name-on-github Jun 16, 2025

Choose a reason for hiding this comment

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

do we have any tests for invalid identifiers? if not we should add one

Copy link
Contributor Author

@7nik 7nik Jun 16, 2025

Choose a reason for hiding this comment

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

The name should be kept as is for non-dynamic components.
In the case of dynamic components, the name just defines the name of an intermediate variable that was shadowing the component's one. And before #15118 it was always $$component. Here, the name only defines a more convenient stack tracing. The regex was there because the component name can be Object.prop, see component-namespace and component-namespaced tests.

I thought about separating the names of the component's source and the intermediate variable, but couldn't come up with a nice one. I can try again tomorrow.

Copy link
Member

@paoloricciuti paoloricciuti Jun 16, 2025

Choose a reason for hiding this comment

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

What if instead we just context.scope.generate(node.name)?

? '$$component_' + node.name.replaceAll(/[^a-zA-Z_$0-9]/g, '_')
: node.name,
context
);
context.state.init.push(component);
Expand Down
5 changes: 5 additions & 0 deletions packages/svelte/tests/runtime-runes/samples/dynamic-component-nested/A.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<script>
const { children } = $props()
</script>

{@render children()}
8 changes: 8 additions & 0 deletions packages/svelte/tests/runtime-runes/samples/dynamic-component-nested/_config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { test } from '../../test';
import { flushSync } from 'svelte';

export default test({
async test({ assert, target }) {
assert.htmlEqual(target.innerHTML, 'test');
}
});
9 changes: 9 additions & 0 deletions packages/svelte/tests/runtime-runes/samples/dynamic-component-nested/main.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<script>
import A from './A.svelte';

const B = $derived(A);
</script>

<B>
<B>test</B>
</B>