Skip to content

chore: consolidate checks for never-static attributes #14372

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
dummdidumm merged 1 commit into from
Nov 20, 2024
Merged

chore: consolidate checks for never-static attributes #14372

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 2 additions & 4 deletions packages/svelte/src/compiler/phases/2-analyze/visitors/RegularElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/** @import { AST } from '#compiler' */
/** @import { Context } from '../types' */
import { is_mathml, is_svg, is_void } from '../../../../utils.js';
import { cannot_be_set_statically, is_mathml, is_svg, is_void } from '../../../../utils.js';
import {
is_tag_valid_with_ancestor,
is_tag_valid_with_parent
Expand Down Expand Up @@ -77,9 +77,7 @@ export function RegularElement(node, context) {

if (
node.attributes.some(
(attribute) =>
attribute.type === 'Attribute' &&
(attribute.name === 'autofocus' || attribute.name === 'muted')
(attribute) => attribute.type === 'Attribute' && cannot_be_set_statically(attribute.name)
Copy link
Contributor

@trueadm trueadm Nov 20, 2024

Choose a reason for hiding this comment

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

This logic can move to Attribute visitor rather than be here.

Copy link
Member Author

@dummdidumm dummdidumm Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I just strictly did replace the usage sites, not do any correctness fixes. This is meant to be a consolidation PR, nothing else

)
) {
mark_subtree_dynamic(context.path);
Expand Down
4 changes: 2 additions & 2 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 @@ -5,6 +5,7 @@
/** @import { Scope } from '../../../scope' */
import { escape_html } from '../../../../../escaping.js';
import {
cannot_be_set_statically,
is_boolean_attribute,
is_dom_property,
is_load_error_element,
Expand Down Expand Up @@ -262,8 +263,7 @@ export function RegularElement(node, context) {

if (
!is_custom_element &&
attribute.name !== 'autofocus' &&
attribute.name !== 'muted' &&
!cannot_be_set_statically(attribute.name) &&
(attribute.value === true || is_text_attribute(attribute))
) {
const name = get_attribute_name(node, attribute);
Expand Down
3 changes: 2 additions & 1 deletion packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/fragment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
/** @import { Scope } from '../../../../scope.js' */
/** @import { ComponentContext } from '../../types' */
import { escape_html } from '../../../../../../escaping.js';
import { cannot_be_set_statically } from '../../../../../../utils.js';
import { is_event_attribute } from '../../../../../utils/ast.js';
import * as b from '../../../../../utils/builders.js';
import { build_template_chunk, build_update } from './utils.js';
Expand Down Expand Up @@ -142,7 +143,7 @@ function is_static_element(node) {
return false;
}

if (attribute.name === 'autofocus' || attribute.name === 'muted') {
if (cannot_be_set_statically(attribute.name)) {
Copy link
Contributor

@trueadm trueadm Nov 20, 2024

Choose a reason for hiding this comment

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

Do we need this, I think we can move this entirely?

return false;
}

Expand Down
11 changes: 11 additions & 0 deletions packages/svelte/src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,17 @@ export function is_dom_property(name) {
return DOM_PROPERTIES.includes(name);
}

const NON_STATIC_PROPERTIES = ['autofocus', 'muted'];

/**
* Returns `true` if the given attribute cannot be set through the template
* string, i.e. needs some kind of JavaScript handling to work.
* @param {string} name
*/
export function cannot_be_set_statically(name) {
return NON_STATIC_PROPERTIES.includes(name);
}

/**
* Subset of delegated events which should be passive by default.
* These two are already passive via browser defaults on window, document and body.
Expand Down
Loading