Skip to content
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

feat: attachments (alternative syntax) #15045

Closed
wants to merge 20 commits into from

Conversation

jrmajor
Copy link
Contributor

@jrmajor jrmajor commented Jan 17, 2025

Alternative to #15000, with simplified syntax.

In Svelte 5, event handler syntax was changed from special on: directives to normal props, which allowed them to be forwarded from a component in more explicit and understandable way, while also simplifying the syntax. I think the same treatment can be applied to actions/attachments:

-<div {@attach (node) => console.log(node)}>...</div>
+<div attachments={[(node) => console.log(node)]}>...</div>

-<button {@attach tooltip('Hello')}>
+<button attachments={[tooltip('Hello')]}>
   Hover me
 </button>

This syntax makes some edge cases mentioned in discussion, as #15000 (comment), trivial to solve:

 <script>
-  import { isAttachmentKey } from 'svelte/attachments';
-
-  let props = $props();
+  let { attachments, ...props } = $props();
-
-  function filter(object, fn) {
-    return Object.fromEntries(Object.entries(object).map(([key, value]) => fn(key, value)));
-  }
 </script>

-<label {...filter(props, (key) => isAttachmentKey(key))}>
-  <input {...filter(props, (key) => !isAttachmentKey(key))}>
+<label {attachments}>
+  <input {...props}>
 </label>

It also makes it easier to correctly type props of a component that accepts attachments (the [key: symbol] syntax isn't super common, I wouldn't be surprised if some devs weren't familiar with it):

 <script lang="ts">
-  let { name, ...attachments }: {
+  let { name, attachments }: {
     name: string,
-    [key: symbol]: (node: HTMLInputElement) => void | (() => void),
+    attachments: Array<(node: HTMLInputElement) => void | (() => void)>,
   } = $props();
 </script>

-<input {name} {...attachments}>
+<input {name} {attachments}>

Since this version doesn't introduce new syntax, it will require no changes to language tools and Prettier / ESLint plugins.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented Jan 17, 2025

🦋 Changeset detected

Latest commit: 0d7f022

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Minor

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

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-svelte-15045-svelte.vercel.app/

this is an automated message

Copy link
Contributor

Playground

pnpm add https://pkg.pr.new/svelte@15045

@Ocean-OS
Copy link
Contributor

Ocean-OS commented Jan 17, 2025

This seems like a bad idea, because:

  1. attachments would be a reserved prop keyword like children
  2. There isn't any sort of characteristic that shows how attachments is different from any other prop
  3. The syntax for attachments is already somewhat agreed on
  4. The current syntax for attachments is more declarative and easier to reason about— you're attaching this function to an element, but here it's more like you're adding this function to the element's attachments

@paoloricciuti
Copy link
Member

This seems like a bad idea, because:

  1. attachments would be a reserved prop keyword like children
  2. There isn't any sort of characteristic that shows how attachments is different from any other prop
  3. The syntax for attachments is already somewhat agreed on

Yeah i'm sorry you did the work to revert stuff and add the new syntax but this is not the right idea. Thanks for contributing anyway. I'm gonna close this 😄

@jrmajor
Copy link
Contributor Author

jrmajor commented Jan 17, 2025

  1. attachments would be a reserved prop keyword like children

@Ocean-OS @paoloricciuti That's not true. This is similar to how event listeners work. By convention, all props beginning with “on” are event listeners, but you don't have to follow that convention.

https://svelte.dev/playground/b71cb484016143ea8a0146b9cd220117?version=pr-15045

  1. There isn't any sort of characteristic that shows how attachments is different from any other prop

Because they are not. That's the point :)

  1. The syntax for attachments is already somewhat agreed on

I feel like most of the discussion in #15000 is about syntax. The most recent comments are about changing syntax to use a different character. I wanted to make a proposal too, but since it's a bigger change, I actually implemented a POC to make sure I don't waste anybody's time with something that wouldn't work.

  1. The current syntax for attachments is more declarative and easier to reason about— you're attaching this function to an element, but here it's more like you're adding this function to the element's attachments

You're already attaching event handlers with plain attributes like onsometing={handler}, so it would make sense to attach attachments the same way. I would argue it's easier to reason about, because it follows existing patterns.

@webJose
Copy link
Contributor

webJose commented Jan 17, 2025

This PR has a good point. I'll make the floatiing-ui POC using this PR....

@paoloricciuti
Copy link
Member

Btw attachments also can't be used since it's not safe from html spec future attributes.

@webJose
Copy link
Contributor

webJose commented Jan 17, 2025

Btw attachments also can't be used since it's not safe from html spec future attributes.

Agreed. This works because whatever property name we choose ("attachments" currently) is not an existing attribute name in any HTML element in existence right now. What if it appears in the future?

But, @paoloricciuti, isn't this the same with events? They are set up by name convention currently. What makes events different? I'm having a hard time pinpointing the difference.

@paoloricciuti
Copy link
Member

Events are actually already attributes on html element and they are basically used to do the same thing.

If tomorrow a brand new browser api comes out and attachments is used to espresso the attachment of the popover API (just a random example) then we would have no way to fix this without a breaking change...that's no bueno.

@webJose
Copy link
Contributor

webJose commented Jan 17, 2025

Hmm, ok. What about @attachments as property name? Possible? Would the addition of the special character give us enough security for the future?

@jrmajor
Copy link
Contributor Author

jrmajor commented Jan 17, 2025

Btw attachments also can't be used since it's not safe from html spec future attributes.

@paoloricciuti That's a valid point, but I think there is a solution to that. Due to how HTML works, all HTML attributes must be strings, and attachments must be JS functions. If such attribute was added to HTML, we could check attachments type, and determine whether it's attachment or element attribute based on its type. It's not a perfect solution, so I wouldn't suggest that if this had high likelihood of collision with future HTML spec. However, I think that attachments is a name that isn't likely to be used for an attribute name, and if it does, we can handle that, so I think it's acceptable.

@kran6a
Copy link

kran6a commented Jan 17, 2025

What about @attachments as property name?

Technically not even the {@attach} proposal is safe as that is a spec-compliant HTML attribute but my guess is that @attachments is as safe to use as {@attach}. Even the spec states that attribute names may (not must) be written with any mix of ASCII lower and ASCII upper alphas despite allowing almost every character.
Personally I would prefer @attachments={[]} syntax over the OG proposal one as it is not confused with other {@something} elements which can't be used within the opening tag of an element, it looks very similar to what we already have with actions and to how event listeners are attached to elements.

@Ocean-OS
Copy link
Contributor

Technically not even the {@attach} proposal is safe as that is a spec-compliant HTML attribute but my guess is that @attachments is as safe to use as {@attach}. Even the spec states that attribute names may (not must) be written with any mix of ASCII lower and ASCII upper alphas despite allowing almost every character.

The @attach syntax isn't spec compliant because it isn't treated in the same way as an attribute. For example, this Svelte code:

<div {@attach thing}></div>

...would be interpreted by the browser as...

<div {@attach="" thing}=""></div>

And lambdas would be even worse:

<div {@attach="" (node)="" =="">{...attachment stuff...}></div> <!--something like that-->

@webJose
Copy link
Contributor

webJose commented Jan 17, 2025

Well, if @attachment (or @attach, perhaps?) is as safe as {@attach fn}, I think I prefer this PR's syntax.

Maybe Svelte can start reserving property names that start with "@"? For future @transition, for example?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants