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: allow usage of $props.id everywhere if invoked within a component script #15295

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

paoloricciuti
Copy link
Member

For the moment is just an exploration but this PR allows for $props.id to be used within utility functions as long as it's invoked within the script of a component.

It works by introducing a setup function that stores the payload on the server and returns a cleanup function that prepend the hydration marker if the function has been used (names are up for debate). On the client does the same to hydrate that marker.

Let's see if we like this or not.

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 Feb 14, 2025

🦋 Changeset detected

Latest commit: 665ae91

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

@paoloricciuti paoloricciuti changed the title Props id anywhere feat: allow usage of $props.id everywhere if invoked within a component script Feb 14, 2025
@svelte-docs-bot
Copy link

Copy link
Contributor

Playground

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

@Rich-Harris
Copy link
Member

I really don't think we should do this. The stated purpose is to allow utility functions to call $props.id(), so that you can do this sort of thing:

<script>
  import { createFoo } from './utils.svelte.js';

  const foo1 = createFoo();
  const foo2 = createFoo();
</script>

<!-- try clicking on the labels to select the input -->
<label {...foo1.label}>click me</label>
<input {...foo1.input} />

<label {...foo2.label}>or me</label>
<input {...foo2.input} />

If createFoo calls $props.id(), then there are two possibilities — the returned value is the same for each call within the component (i.e. this PR's current behaviour), or there's a per-component incrementer in addition to the global one. Both are bad.

If the value is the same each time, then the example above would be subtly broken in a way that would be easy to accidentally ship to production, and which would be difficult to diagnose since the call to $props.id() is hidden behind an abstraction.

If the value is different each time, then the IDs are no longer stable:

<!--stable -->
<p>{$props.id()}</p>

<!-- unstable -->
{#if browser}
  <p>{$props.id()}</p>
{/if}

<p>{$props.id()}</p>

Other frameworks accept instability as the price of being able to generate a unique ID anywhere (or rather, they don't have the ability to constrain it like we do), but it seems like a bad trade-off. Utility functions can simply accept an ID that defaults to crypto.randomUUID(), allowing the user to solve the problem themselves, using $props.id() with a local incrementer if they need hydration-stable IDs.

@paoloricciuti
Copy link
Member Author

paoloricciuti commented Feb 14, 2025

I think the point is that props id is the ONLY way to create a server+client stable id. If an utility can accept an unstable id too is fine but if there's an utility that relies on the id being stable then the utility would have to hope the user does the right thing and uses $props.id.

IMHO once the fact that the returned Id is documented and common knowledge no one would use it to create something like this.

@adiguba
Copy link
Contributor

adiguba commented Feb 14, 2025

Hello,

I don't see the use-case for that.

If you need the id on a utility function, you can simply pass it as parameter :

const uid = $props.id();
const foo1 = createFoo(uid);
const foo2 = createFoo(uid);

@Rich-Harris
Copy link
Member

no one would use it to create something like this.

To be absolutely clear: you cannot use $props.id() inside an abstraction if it returns the same value each time, without the abstraction being buggy. It's a non-starter. The only way this could work is if the returned value is unstable, but if stability isn't important then you're fine defaulting to crypto.randomUUID() anyway. Either way, there's just no use case for this that I can see.

@paoloricciuti
Copy link
Member Author

As i've said this was just an exploration so we can close this...but I can't stress enough that the point of being able to use it in an utility is exactly when the utility absolutely needs a stable reference. Passing it from outside it's a non option because there's no guarantee it will be stable.

Nothing stops the user of the library to do something like this

const uid = crypto.randomUUID();
const foo1 = createFoo(uid);
const foo2 = createFoo(uid);

and if createFoo is relying on the id being stable it's done.

I think this is a much stronger use case than creating utilities like in melt. But again maybe is too little of an edge case for it to be worth this PR.

@paoloricciuti
Copy link
Member Author

without the abstraction being buggy

i just think it depends on the kind of abstraction...if it's to create a melt like abstraction i agree. But that's not the only use case.

@Rich-Harris
Copy link
Member

What's a use case where it would be okay to reuse a unique ID?

@paoloricciuti
Copy link
Member Author

What's a use case where it would be okay to reuse a unique ID?

When you have something that would "interact" with server side rendering. For example imagine an utility like this

function store_component_state(my_state){
    let id = $props.id();
    // pretend create raw snippets works like this
    let snippet = createRawSnippet(`
        <script>
            if(!window.states) window.states = new Map();
            if(!window.states.has('${id}')) window.states.set('${id}', []);
            window.states.get('${id}').push(my_state);
        </script>
    `);

    return snippet;
}

Obviously you could then load this info in another script and do something with it. In this case the id needs to be unique per component or it will get mangled with other instances. Personally i've used tricks like this multiple times (for example to make a dialog works before the whole bundle is downloaded) and i've always resorted to let the user of the component specify an id manually and hope the two would not conflict.

Again I know this is a veeeeery niche use case but currently this is not possible at all today

@paoloricciuti
Copy link
Member Author

And if I know that $props.id will return the same id everytime and i need it to not do it i can always implement an internal counter myself if i know it doesn't make sense for the user to conditionally invoke my utility.

@Ocean-OS
Copy link
Contributor

To reduce compiled output, couldn't $.setup be called in $.push, and $$cleanup in $.pop()? This probably would involve adding $$cleanup as a property of the component context.

@paoloricciuti
Copy link
Member Author

To reduce compiled output, couldn't $.setup be called in $.push, and $$cleanup in $.pop()? This probably would involve adding $$cleanup as a property of the component context.

Push and pop are only added if the component needs context.

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

Successfully merging this pull request may close these issues.

4 participants