-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
chore: small tweaks on set_attributes() #15413
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.
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
Conversation
🦋 Changeset detectedLatest commit: 346de0e 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 |
|
? b.literal(context.state.analysis.css.hash) | ||
: is_custom_element | ||
? b.null | ||
: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's this change about? Why null
in one case and false
in the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just to generate a null
instead of undefined
for custom-element :
$.set_attributes(cust_elem, attributes_1, { ...rest }, undefined, true, true);
// =>
$.set_attributes(cust_elem, attributes_1, { ...rest }, null, true, true);
But maybe it's useless...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to keep it as undefined
— there's probably a bunch of places where we have these arguments that are omitted in favour of defaults if there are no subsequent non-falsy arguments, and if we always added this logic it would make the codebase more cluttered. With gzip the extra characters won't matter
Co-authored-by: Simon H <5968653+dummdidumm@users.noreply.github.com>
…ear in changelog
packages/svelte/src/compiler/phases/3-transform/client/visitors/shared/element.js
Outdated
Show resolved
Hide resolved
…s/shared/element.js
Just a question : what the good value for preserve_attribute_case ? Like I said currently it's true for custom-element, SVG or MathML on RegularElement, but only on SVG for svelte:element. I use the value on RegularElement in this PR, but I'm not sure if that's correct... |
Ah, yep. Attributes are case-sensitive on everything except regular (non-custom) HTML elements IIUC, so the check should probably be that instead. Looking more closely at some compiled output I wonder if we should do that check inside |
merged #15443, so I'll close this PR |
Changes :
skip_warning
ofset_attributes()
was unused : now it's passed toset_attribute()
.preserve_attribute_case
for<svelte:element>
is now consistent with regular tags : it's true for custom elements, SVG or MathML (note : is this correct?)<svelte:element>
, the value ofpreserve_attribute_case
andis_custom_element
are now evaluated when the node is created inside$.element()
, and no longer at each update of the template.Example of generated code :
Note : I don't known how to test that...
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 test
and lint the project withpnpm lint