fix: avoid shadowing a variable in dynamic components #16185
Conversation
🦋 Changeset detectedLatest commit: 4d0794a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
| // 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
do we have any tests for invalid identifiers? if not we should add one
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What if instead we just context.scope.generate(node.name)?
Fixes #16163 caused by regression in #16118
Before submitting the PR, please make sure you do the following
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint