fix: ReferenceError: $0 is not defined #15317
Conversation
🦋 Changeset detectedLatest commit: 817cf1a 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 |
|
| /** @import { SourceLocation } from '#shared' */ | ||
| /** @import { ComponentClientTransformState, ComponentContext } from '../types' */ | ||
| /** @import { Scope } from '../../../scope' */ | ||
| import { escape_html } from '../../../../../escaping.js'; |
There was a problem hiding this comment.
| import { escape_html } from '../../../../../escaping.js'; |
This doesn't seem to be used right?
| get_attribute_name, | ||
| build_attribute_value, | ||
| build_class_directives, | ||
| build_set_attributes, |
There was a problem hiding this comment.
| build_set_attributes, |
This doesn't seem to be used right?
| build_set_attributes, | ||
| build_style_directives, | ||
| build_set_attributes | ||
| get_attribute_name |
There was a problem hiding this comment.
| get_attribute_name |
This doesn't seem to be used right?
| build_set_attributes | ||
| get_attribute_name | ||
| } from './shared/element.js'; | ||
| import { visit_event_attribute } from './shared/events.js'; |
There was a problem hiding this comment.
| import { visit_event_attribute } from './shared/events.js'; |
This doesn't seem to be used right?
| ); | ||
|
|
||
| if (is_select_with_value) { | ||
| let { value } = build_attribute_value(attribute.value, context); |
There was a problem hiding this comment.
Mmm i don't think this is the right fix: this is basically doing the same thing it would do anyway without checking if metadata.has_call to get_expression_id.
I think a more thoughtful solution would be to figure out why the expression is not there (most likely we need to create a $0 which is a derived). Let me try to take a look so that i can guide you better.
There was a problem hiding this comment.
Ok i've explored this a bit more and there are a couple of things:
- we could fix it with something similar to this but instead of rerunning
build_attribute_valueI would just return thenon_memoizedvalue from the function too and use that...this lead me to the next point. - This is not the only place where a bug similar to this exists. For example if you do something like this
<input autofocus={should_focus()} />it will also error out so we should fix this too.
However fixing it this way could lead to an unexpected behaviour where the function will be called twice the first time (once to initialise it and once in the template effect). So i wonder if we should find a way to access the expressions and pass them in from autofocus and init_select.
P.s. we should also add a test for both this cases.
There was a problem hiding this comment.
Ok so the problematic ones are
- autofocus
- muted
- select value
i think the best course of action in this case would be to check within the memoize function passed as argument to build_attribute_value if there's the need for this functions and in that case instead of calling get_expression_id we return initialize add a new derived to the init and return that id from the memoize.
The downside of this is that when we then pass that derived to template_effect we would be unnecessarily create a derived of a derived but i think it's probably fine.
|
@jhwz do you have capacity to fix this properly? I think we should try to fix it asap |
|
I think it's a bit out of my depth, I'm not familiar enough with the codebase. Feel free to close this and open another PR. |
It's close enough to your solution if you want to try...I don't know when I'll have time to open another pr if you want to try in the meantime. Otherwise I'll try to open a new PR adding you as co author 😉 |
|
I'll see what I can do! |
This reverts commit 2c5965c.
2 participants
Before submitting the PR, please make sure you do the following
closes #15316
Not super efficient because we're duplicating the expression, the actual issue is likely something else entirely! I'm not at all familiar with the codebase so not going to look much further but this may give you a start.
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint