Skip to content

fix: ReferenceError: $0 is not defined #15317

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

Closed
wants to merge 5 commits into from
Closed

fix: ReferenceError: $0 is not defined #15317

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2c5965c
probably not the best fix but oh well
jhwz Feb 17, 2025
7a30a54
add changeset
jhwz Feb 17, 2025
1986493
Revert "probably not the best fix but oh well"
jhwz Feb 17, 2025
7be381c
chore: add failing test
jhwz Feb 17, 2025
817cf1a
chore: add all failing cases to test
jhwz Feb 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/shiny-shoes-yell.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'svelte': patch
---

Fix ReferenceError bug (#15316)
9 changes: 5 additions & 4 deletions packages/svelte/src/compiler/phases/3-transform/client/visitors/RegularElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,35 +3,35 @@
/** @import { SourceLocation } from '#shared' */
/** @import { ComponentClientTransformState, ComponentContext } from '../types' */
/** @import { Scope } from '../../../scope' */
import { escape_html } from '../../../../../escaping.js';
Copy link
Member

@paoloricciuti paoloricciuti Feb 17, 2025

Choose a reason for hiding this comment

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

Suggested change
import { escape_html } from '../../../../../escaping.js';

This doesn't seem to be used right?

import {
cannot_be_set_statically,
is_boolean_attribute,
is_dom_property,
is_load_error_element,
is_void
} from '../../../../../utils.js';
import { escape_html } from '../../../../../escaping.js';
import { dev, is_ignored, locator } from '../../../../state.js';
import { is_event_attribute, is_text_attribute } from '../../../../utils/ast.js';
import * as b from '../../../../utils/builders.js';
import { is_custom_element_node } from '../../../nodes.js';
import { clean_nodes, determine_namespace_for_children } from '../../utils.js';
import { build_getter } from '../utils.js';
import {
get_attribute_name,
build_attribute_value,
build_class_directives,
build_set_attributes,
Copy link
Member

@paoloricciuti paoloricciuti Feb 17, 2025

Choose a reason for hiding this comment

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

Suggested change
build_set_attributes,

This doesn't seem to be used right?

build_style_directives,
build_set_attributes
get_attribute_name
Copy link
Member

@paoloricciuti paoloricciuti Feb 17, 2025

Choose a reason for hiding this comment

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

Suggested change
get_attribute_name

This doesn't seem to be used right?

} from './shared/element.js';
import { visit_event_attribute } from './shared/events.js';
Copy link
Member

@paoloricciuti paoloricciuti Feb 17, 2025

Choose a reason for hiding this comment

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

Suggested change
import { visit_event_attribute } from './shared/events.js';

This doesn't seem to be used right?

import { process_children } from './shared/fragment.js';
import {
build_render_statement,
build_template_chunk,
build_update_assignment,
get_expression_id
} from './shared/utils.js';
import { visit_event_attribute } from './shared/events.js';

/**
* @param {AST.RegularElement} node
Expand Down Expand Up @@ -693,6 +693,7 @@ function build_element_special_value_attribute(element, node_id, attribute, cont
);

if (is_select_with_value) {
let { value } = build_attribute_value(attribute.value, context);
Copy link
Member

@paoloricciuti paoloricciuti Feb 17, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@paoloricciuti paoloricciuti Feb 17, 2025

Choose a reason for hiding this comment

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

Ok i've explored this a bit more and there are a couple of things:

  1. we could fix it with something similar to this but instead of rerunning build_attribute_value I would just return the non_memoized value from the function too and use that...this lead me to the next point.
  2. 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.

Copy link
Member

@paoloricciuti paoloricciuti Feb 17, 2025

Choose a reason for hiding this comment

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

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.

state.init.push(b.stmt(b.call('$.init_select', node_id, b.thunk(value))));
}

Expand Down