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

alternative attachments syntax #15076

Open
wants to merge 4 commits into
base: attachments
Choose a base branch
from
Open

alternative attachments syntax #15076

wants to merge 4 commits into from

Conversation

Rich-Harris
Copy link
Member

alternative syntax for #15000:

<canvas
  class="cool"
  width={64}
  height={64}
  attach((node) => {
    // ...
  })
></canvas>

<input attach(autoselect) />

Copy link

changeset-bot bot commented Jan 21, 2025

⚠️ No Changeset found

Latest commit: 35b9d21

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

Playground

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

@webJose
Copy link
Contributor

webJose commented Jan 21, 2025

There seems to be a performance issue the first time attachments are used.

The following are REPL's that do the same thing: It fixes the position of the caret when function binding is used to format an input's value.

REPL using extra bindable prop
REPL using attachments <--- CORRECTED LINK

The UI gets stuck temporarily when you start typing using the attachments variant.

@webJose

This comment was marked as outdated.

@7nik
Copy link

7nik commented Jan 21, 2025

it should be ?version=pr-15076 (your is ?version=pr--15076) and the syntax is attach(getInputEl)

@webJose
Copy link
Contributor

webJose commented Jan 21, 2025

Apologies for the mistake. I did not save the REPL because it is linked to a StackOverflow answer that references PR 15.000.

New LINK

@Leonidaz
Copy link

would this work? The pr currently only supports explicit inline "declaration", e.g. <input attach(func, func, func)>

let arr = [
  (node) => console.log('a'),
  (node) => console.log('b'),
  (node) => console.log('c')
];

<input attach(arr)> or <input attach(...arr)> or <input attach((node) => {}, ...arr)>

@Rich-Harris
Copy link
Member Author

@Leonidaz fixed

@Leonidaz Leonidaz mentioned this pull request Jan 22, 2025
6 tasks
@Not-Jayden
Copy link
Contributor

Not-Jayden commented Jan 22, 2025

I don't know if this is overly important, but should attachments be expected to be added in a sequentially deterministic order?

This isn't what I expected would happen for this example, but I'm also not certain whether it will matter for any practical use cases 🤷‍♂️

@PuruVJ
Copy link
Collaborator

PuruVJ commented Jan 22, 2025

Gonna advocate for @attach instead of attach for visual distinctness

@gterras
Copy link

gterras commented Jan 22, 2025

Gonna advocate for @attach instead of attach for visual distinctness

Indeed I find a bit worrying that the very special nature of actions isn't acknowledged with this syntax. An action can basically negates an arbitrary amount of Svelte code, without Svelte being notified at all. This should be among the very first things a developer reading a component should be aware of. Now this makes it very hard to scan and/or color highlight.

Stepping outside Svelte realm should come with a warning. This attribute-like syntax heavily normalizes actions, making them a detail of your component, and telling the developer Have all the fun you want with your node!, which probably isn't the best idea.

@Leonidaz
Copy link

should attachments be expected to be added in a sequentially deterministic order

I think they should. I noticed that (node) => {} is prioritized higher than array destructure ...[] Order correct with only (multiple) destructured arrays

@Rich-Harris Incidentally, I really like just having one attach() per element vs allowing multiple. Hopefully it can work for transitions, etc. It feels very clean and aesthetically pleasing to me.

For destructuring, not sure if it's worth it, and not a big deal but wonder if pojos can also be destructured like arrays inside attach()? I can always do ...Reflect.ownKeys(obj).map((k) => obj[k]) but would make life easier if svelte just did it.

attribute-like syntax heavily normalizes actions

@gterras
It sounds like you're saying that it should more difficult for everyone? I don't think it's an issue. If people are coming from other frameworks they don't typically access nodes directly either. If they're newbies, they could reach for this, although it's not something that is emphasized in the docs or tutorials, but I think this is easily remedied by warnings in the documentation.

I think this actually makes svelte more fun to use especially with transitions, animations, etc. as you're not scared away by some special more complicated syntax and instead you can just focus on functionality and js.

@gterras
Copy link

gterras commented Jan 22, 2025

It sounds like you're saying that it should more difficult for everyone?

Adding a @ prefix doesn't make it more difficult for anyone, unless you consider {#if} is more difficult than {if} and so on. The rest of your point seems to focus on writing a component, mine is about maintaining it.

@Leonidaz
Copy link

I think @ is completely unnecessary as attach() is already special syntax. Maybe some special syntax color highlighting can be done as well 🤷‍♂️

I'm not sure I'm following your point on maintenance, especially if there is one one attach() that is allowed per element.

@bennymi
Copy link
Contributor

bennymi commented Jan 24, 2025

I stopped following the other PR because it got a bit too long, so I'm not sure what changes were made. Are attachments still spreadable with this alternative approach? Something like this I mean: Example

@Leonidaz
Copy link

You have to use attach() and at least so far it only takes arrays repl.

I asked about object destructuring earlier #15076 (comment)

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.

8 participants