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

chore: add $derived.call rune #10240

Merged
merged 11 commits into from
Jan 29, 2024
Merged

chore: add $derived.call rune #10240

merged 11 commits into from
Jan 29, 2024

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Jan 19, 2024

Rich here — after extensive discussion we decided to leave out the previous value from the signature, and to call it $derived.call(fn) rather than $derived.fn(fn). Original comment follows:


$derived.fn

Sometimes you need to create complex derivations which don't fit inside a short expression. In this case, you can resort to $derived.fn which accepts a function as its argument and returns its value.

<script>
	let count = $state(0);
	let complex = $derived.fn(() => {
		let tmp = count;
		if (count > 10) {
			tmp += 100;
		}
		return tmp * 2;
	});
</script>

<button on:click={() => count++}>
	{complex}
</button>

$derived.fn passes the previous derived value as a single argument to the derived function. This can be useful for when
you might want to compare the latest derived value with the previous derived value. Furthermore, you might want to pluck out specific properties of derived state to use in the new derived state given a certain condition – which can be useful when dealing with more complex state machines.

<script>
	let items = $state([1, 1, 1]);

	let reversed = $derived.fn((prev) => {
		const reversed = items.toReversed();
		// Naive deep equals comparison
		if (JSON.stringify(reversed) === JSON.stringify(prev)) {
			return prev;
		}
		return reversed;
	});
</script>

<button on:click={() => items.push(1)}>
	{reversed}
</button>

Copy link

changeset-bot bot commented Jan 19, 2024

🦋 Changeset detected

Latest commit: 97261bf

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

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

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

I'm not convinced this is the right API.

Firstly, consider the case where you want to reuse the comparison logic in multiple places. You'd have to do this sort of thing...

<script>
  let one = $state([1, 1, 1]);
  let two = $state([2, 2, 2]);

  function equal(a, b) {
    return JSON.stringify(a) === JSON.stringify(b);
  }

  let one_reversed = $derived.fn((prev) => {
    const reversed = one.toReversed();
    if (equal(reversed, prev)) return prev;
    return reversed;
  });

  let two_reversed = $derived.fn((prev) => {
    const reversed = two.toReversed();
    if (equal(reversed, prev)) return prev;
    return reversed;
  });
</script>

...when this would be much easier:

<script>
  let one = $state([1, 1, 1]);
  let two = $state([2, 2, 2]);

  function equal(a, b) {
    return JSON.stringify(a) === JSON.stringify(b);
  }

  let one_reversed = $derived(one.toReversed(), { equal });
  let two_reversed = $derived(two.toReversed(), { equal });
</script>

Secondly, the fact that prev is undefined at first will often make the logic more cumbersome than it needs to be. At the very least, you'll need to use optional chaining and/or type narrowing in a lot of cases.

Thirdly, it's honestly a bit weird that you signal 'the values are equal' by returning prev. It feels like an accident, rather than deliberate API design.

Fourthly, if equality checking is useful, it's useful for $derived(...) as well as $derived.fn(...).

I'm still not wholly convinced that we need this, but if we really must then I really think we should work on the naming. Elsewhere we prefer to use full words where possible. I'm not sure what a better name would be — will have a think — but I hope we can avoid .fn.

@Rich-Harris
Copy link
Member

I also think it's worth noting that useMemo doesn't take a previous value, and as far as I know this hasn't been a problem for React. Are we sure we need it?

@Not-Jayden
Copy link
Contributor

I'm not convinced this is the right API.

Firstly, consider the case where you want to reuse the comparison logic in multiple places. You'd have to do this sort of thing...

<script>
  let one = $state([1, 1, 1]);
  let two = $state([2, 2, 2]);

  function equal(a, b) {
    return JSON.stringify(a) === JSON.stringify(b);
  }

  let one_reversed = $derived.fn((prev) => {
    const reversed = one.toReversed();
    if (equal(reversed, prev)) return prev;
    return reversed;
  });

  let two_reversed = $derived.fn((prev) => {
    const reversed = two.toReversed();
    if (equal(reversed, prev)) return prev;
    return reversed;
  });
</script>

I mean I figure you'd end up use curried functions for those cases where you would be straight duplicating the logic. i.e.

<script>
  let one = $state([1, 1, 1]);
  let two = $state([2, 2, 2]);

  function equal(a, b) {
    return JSON.stringify(a) === JSON.stringify(b);
  }

  function createReverser(state) {
    return (prev) => {
      const reversed = state.toReversed();
      if (equal(reversed, prev)) return prev;
      return reversed;
    };
  }

  let one_reversed = $derived.fn(createReverser(one));
  let two_reversed = $derived.fn(createReverser(two));
</script>

...when this would be much easier:

<script>
  let one = $state([1, 1, 1]);
  let two = $state([2, 2, 2]);

  function equal(a, b) {
    return JSON.stringify(a) === JSON.stringify(b);
  }

  let one_reversed = $derived(one.toReversed(), { equal });
  let two_reversed = $derived(two.toReversed(), { equal });
</script>

This is definitely more succinct, though it's not immediately obvious to me what is actually happening here. I take it equal a reserved property that accepts a predicate? Could be a nice addition, though it still doesn't address the first consideration of being able to define complex logic inside of $derived without IIFE's.

I do also agree all things considered, the desire to avoid IIFE's is very much a convenience/preference thing. That said there are definitely people with stronger opinions than my own, and using this syntax to provide the prev callback value does add some additional justification on top of that at least.

@dummdidumm
Copy link
Member

example from discord for another potential use case.

@trueadm
Copy link
Contributor Author

trueadm commented Jan 20, 2024

I also think it's worth noting that useMemo doesn't take a previous value, and as far as I know this hasn't been a problem for React. Are we sure we need it?

You'd need to use useRef to store the old value in from experience. Which is cumbersome. Either way, I do think the naming could be resolved for this PR, but having access to the previous value is super invaluable for complex cases. As for equality, we can still have that as dedicated option and change the documentation, however maybe it's not needed unless we plan on providing it for $state too (which might be harder to explain now with deep state).

@Rich-Harris
Copy link
Member

I mean I figure you'd end up use curried functions for those cases

...which is a level of complexity that isn't really in keeping with Svelte. It's also broken — updating one and two doesn't create a new deriver. For that to work, you'd have to put the function inside another function... this feels like a very strong signal that the API is fatally flawed, with or without the prev value. You'd probably need to enforce that the argument to $derived.fn must be a function expression (or arrow function expression) — hello there, uncanny valley.

example from discord for another potential use case.

Not sure I understand. You can just do this?

-const prev = $derived.fn(previousState(() => value))
+const prev = $derived(previousState(() => value)())

So there's two things going on here:

  • using functions instead of expressions — I understand the desire for this, but as shown above there's a real footgun involved, and the mitigations for that footgun (either enforcing function expressions, which reduces reusability, or adding the runtime overhead of tracking whether signals were read while creating the deriver) have drawbacks that we probably don't want
  • passing prev into the deriver — the API is a non-starter in my opinion, for the reasons outlined above. I would love to understand some non-contrived use cases for this. If you really need it, it's solvable in userland (albeit clunkily), but I suspect most of the time it's actually undesirable (equality checking isn't free — it's often quite expensive — and in general you want equality checks to be as far upstream as possible rather than in downstream derivers) so we probably don't want to actively encourage people to use it.

@trueadm trueadm closed this Jan 20, 2024
@dummdidumm
Copy link
Member

I don't think that this kind of mistake in usage hints at a fatal flaw in API design, it's just how the system works. Having access to the previous value is a nice bonus to enable reducer patterns (the equality thing is an accidental side effect of this for me)

@trueadm
Copy link
Contributor Author

trueadm commented Jan 21, 2024

passing prev into the deriver — the API is a non-starter in my opinion, for the reasons outlined above. I would love to understand some non-contrived use cases for this. If you really need it, it's solvable in userland (albeit clunky), but I suspect most of the time it's actually undesirable (equality checking isn't free — it's often quite expensive — and in general you want equality checks to be as far upstream as possible rather than in downstream derivers) so we probably don't want to actively encourage people to use it.

I'm not sure why this is a non-starter TBH. Other than it initially being undefined, which means you can pass a default argument for it, I don't see the problems. It has valid usability as I was trying to point out in https://gist.github.com/trueadm/671dbe4d085cc6d313fcd8074f73a1b7. This works just like it does with createMemo from Solid and that's how I'd like it to work here. Negating the use cases for equality, we can skip that topic and have a dedicated API for it, as it does read better.

If I'm using $derived to generate new $state each time, either via a class or just deep state, then I'm hinting that some state gets updated from above and thus needs to stay in "sync". However, I might also have some state in that derived value that is not designed to be in sync. This is what I demonstrated on https://gist.github.com/trueadm/671dbe4d085cc6d313fcd8074f73a1b7 with the latest changes.

In many ways this is kind of like how you can set state in React with knowledge of the previous values (setState accepts a function that gives you the previous value, so you derive new state given the previous state) – and there's also useReducer that provides the same pattern. I guess what I'm looking for is something that's like this:

<script>
  const { markdown, submitMarkdown } = $props();
  
  let is_editing = $state(false);
  const initial_value = { markdown: '' };
  const state = $derived.reduce(state => {
    let new_state = $state({ markdown: is_editing ? state.markdown : markdown });
    
    return new_state;
  }, initial_value);
</script>

<MarkdownEditor
  bind:markdown={state.markdown}
  onEdit={() => is_editing = true}
  onReadOnly={() => is_editing = false} 
  onSave={() => submitMarkdown(state.markdown)}
/>

Imagine we don't control <MarkdownEditor>, it's some third-party library that expects a bound prop, how can we make this work without using effects today?

It's tempting to try and do something like this:

<script>
  const { markdown, submitMarkdown } = $props();
  
  let is_editing = $state(false);
  let local_state = $state({ markdown });
  const state = $derived(is_editing ? local_state : { markdown });
</script>

However, when we switch modes back and forth, the local_state.markdown will go back to what was there before, rather than the latest version from props. Which seems like a sync issue.

I'm eagerly trying to push people away from this pattern:

https://discord.com/channels/457912077277855764/1153350350158450758/1198567900450144326

Another approach

So I thought to myself, how can we deal with this from another angle? Taking my above example, maybe it makes more sense to position it from an another angle. What if we could derive values from within state?

<script>
  const { markdown, submitMarkdown } = $props();
  
  let is_editing = $state(false);
  
  let state = $state({ markdown }, state => ({ markdown: is_editing ? state.markdown : markdown }));
</script>

@Rich-Harris
Copy link
Member

Rich-Harris commented Jan 21, 2024

Let's at least keep this issue open for now, since the discussion is still ongoing.

I'm not sure why this is a non-starter TBH. Other than it initially being undefined, which means you can pass a default argument for it, I don't see the problems.

I outlined a few points above, but consider also the impact on types:

image

As soon as prev becomes a potential return value, you can no longer rely on inference. An explicit equal option, were we to add such a thing, wouldn't have that drawback.

I think the reducer idea is interesting. Initially it seemed off because the reducer pattern is (state, action) => state and there's no action here — nothing gets dispatched. But you could think of the action as being implicit. I think this idea is worth exploring further, and trying it out in different scenarios.

I'm eagerly trying to push people away from this pattern:

https://discord.com/channels/457912077277855764/1153350350158450758/1198567900450144326

For posterity:

// data comes from sveltekit load function
const { data, form } = $props()

let markdown = $state(data.markdown)
$effect(() => {
  markdown = data.markdown
})

If we were to use bind:data inside SvelteKit to enable optimistic UI (#9905), this would work. That's one option, though it's not a general solution to the 'I have some state that I want to be able to edit locally' problem.

We need to fully articulate that problem though. If it's 'I want the view of some state to be this value while some condition (e.g. is_editing) is true, and this other value when it's false' then that's one thing (and something that can be solved today).

But if it's 'I want to have some locally edited state that is replaced whenever there's new stuff from the server' — which better describes the above snippet — then it seems to me that effects are the right tool for the job, because fundamentally what you're trying to do is have a piece of state that represents the most recent of <data from server> and <local edits>, and effects are how you incorporate temporal concepts like recency. I don't think a reducer pattern helps us here.

@Rich-Harris Rich-Harris reopened this Jan 21, 2024
@trueadm
Copy link
Contributor Author

trueadm commented Jan 21, 2024

But if it's 'I want to have some locally edited state that is replaced whenever there's new stuff from the server' — which better describes the above snippet — then it seems to me that effects are the right tool for the job, because fundamentally what you're trying to do is have a piece of state that represents the most recent of and , and effects are how you incorporate temporal concepts like recency. I don't think a reducer pattern helps us here.

One of things that I'm worried about is a future in where we have the forking of state. For example, a pending state tree for async/suspenseful transitioning UI that has not yet been applied. In a fork that is still pending, we cannot run effects – as they might mutate the DOM or trigger some other irreversible side-effect. If there is some effect that is responsible for syncing the state tree then that won't happen anymore. This is important, as an incoming prop may have changed, causing the pending state to change in some part. If that no longer happens then you have a glitchy state tree. So moving as much of that to derived state is always preferable.

@Rich-Harris
Copy link
Member

Ha - I was also thinking about that stuff, but from a different angle. Namely that the smaller the API surface area today, the more freedom we'll have in future. Part of the reason I'm so resistant to new API in general is that I've often been painted into a corner by convenience APIs that weren't truly necessary.

Big picture: we're on the same page, just not fully aligned on all the details

@mimbrown
Copy link
Contributor

mimbrown commented Jan 22, 2024

using functions instead of expressions — I understand the desire for this, but as shown above there's a real footgun involved, and the mitigations for that footgun (either enforcing function expressions, which reduces reusability, or adding the runtime overhead of tracking whether signals were read while creating the deriver) have drawbacks that we probably don't want

I don't really understand this "adding the runtime overhead of tracking whether signals were read while creating the deriver."

If $derived(expr) compiles to $.derived(() => expr), then $derived.fn(function) would compile to $.derived(function) (which in my experience is how most people think about it anyway), and it doesn't matter whether the function is defined inline or elsewhere. The only way I could think of to use a signal while creating a deriver would be to use an IIFE inside derived.fn, which... kinda feels like the dev probably knows what they're doing at that point.

EDIT: I take that back, I understand that there's multiple ways that signals could be read while creating a deriver:

const myDerived = $derived.fn(higherOrderFunction());
const myDerived = $derived.fn(someCondition ? functionOne : functionTwo);

But I don't think it's cumbersome to explain that "only signals read inside the passed function will be tracked." I stand by that many people already think in those terms.

The ability to pass the function itself opens up possibilities like when you want to do something with feature detection, where you want the ability to pass completely different logic based on some check that only needs to happen once, like:

const myDerived = $derived.fn(browserSupportsFeatureX ? functionOne : functionTwo);

@Thiagolino8
Copy link

Not sure I understand. You can just do this?

-const prev = $derived.fn(previousState(() => value))
+const prev = $derived(previousState(() => value)())

The example given by @dummdidumm was mine
The reason I didn't use $derived but rather $derived.fn for the job was that I didn't know it was possible
Because honestly it is not at all intuitive that $derived has two different behaviors, most of the time re-executing the entire passed expression but re-executing only the return of a function if that return is called
For me a consistent behavior would be that if const c = $derived(a * 2) compiles to const c = $.derived(() => a * 2), then const c = $derived(double(() => a)()) should compile to const c = $.derived(() => double(() => a)()) not const c = $.derived(double(() => a))
In my opinion the problem is not that $derived.fn is doing something that $derived can already do, but that $derived is doing too many things
As for the name, to maintain consistency with $inspect.with I think $derived.with would be ideal

@aradalvand
Copy link

I second the naming $derived.with over $derived.fn

@dummdidumm
Copy link
Member

$inspect.with has nothing in common with derived semantically (the former reads like "inspect this value with that callback function", which $derived.with doesn't), and as such fn as a shorthand for function makes much more sense to me.

@navorite
Copy link
Contributor

Does it have to be a separate API though, at least if there are no additional changes other than it being a function? Can't it just be overloaded and have the type checked in the compilation step?

@Rich-Harris
Copy link
Member

But I don't think it's cumbersome to explain that "only signals read inside the passed function will be tracked." I stand by that many people already think in those terms.

I was making that point in response to @Not-Jayden's comment upthread...

I mean I figure you'd end up use curried functions for those cases where you would be straight duplicating the logic

...which wouldn't work. The very first example of someone using $derived.fn (and I'm not picking on @Not-Jayden here, it's a very easy error to make!) contained a subtle bug (subtle because it might not even fail until you refactor your code that mutates one to code that reassigns one). If you need to carefully read through the documentation and deeply understand the implementation in order to suppress your intuitions about how things should work, then the API is flawed.

That's why I'd be slightly more open to a version of this where you can only pass a function expression:

const ok = $derived.whatever(() => {...});
const not_ok = $derived.whatever(somefn);

It would prevent that category of bugs, at the cost of the 'uncanny valley' phenomenon.

.with definitely isn't the right name for this. One idea: $derived.block(() => {...})?

Does it have to be a separate API though, at least if there are no additional changes other than it being a function? Can't it just be overloaded and have the type checked in the compilation step?

Would love it if that was possible, but I don't think it is. Consider:

const filter_function = $derived(filter_functions[current_filter]);

How would you type that such that filter_function was a function and not the result of calling filter_function?

@Thiagolino8
Copy link

Does it have to be a separate API though, at least if there are no additional changes other than it being a function? Can't it just be overloaded and have the type checked in the compilation step?

This was my first suggestion #9984, which was closed in favor of #9968

How would you type that such that filter_function was a function and not the result of calling filter_function?

In case of overloading, the correct return should be the result of calling filter_function
Just make it clear that if the value passed to $derived is a function then the value returned will be the result of this function
If you want the returned value to be the function itself, then just return it from a callback

const filter_result = $derived(filter_functions[current_filter]);
const filter_function = $derived(() => filter_functions[current_filter]);

@Rich-Harris
Copy link
Member

We spent a healthy chunk of time discussing this issue on Friday, and reached the following conclusions:

  • adding a variant that takes a function shouldn't prevent us from implementing async state, when the time comes
  • we want to keep $derived(expression)
  • therefore the variant should be a new sub-rune. we settled on $derived.call(fn) as the name
  • we don't want to pass the previous value into the function

@Rich-Harris Rich-Harris merged commit d9a6b8b into main Jan 29, 2024
8 checks passed
@Rich-Harris Rich-Harris deleted the derived-fn branch January 29, 2024 17:43
@Not-Jayden
Copy link
Contributor

Not-Jayden commented Jan 30, 2024

I know this is probably too little too late to flag this, but I intentionally avoided suggesting call as the method name as it conflicts with the reserved Function.prototype.call() method for functions in JS.

e.g. in theory you would expect

$derived.call(null, count * 2);

to be equivalent to:

$derived(count * 2);

While I don't think its a huge deal, it is breaking JS conventions a little.

@elliott-with-the-longest-name-on-github
Copy link
Contributor

IMO the above is a nonissue; $derived is already breaking function semantics in that the expression inside of it is reevaluated when dependencies change, rather than being executed and “passed into” the “function”. It’s pretty clear beginning-to-end that runes are not functions and don’t hold to JavaScript function semantics.

@eltigerchino
Copy link
Member

Furthermore, it’s been mentioned before that runes are meant to be similar to the import() keyword. So, the conventional function rules don’t apply:

The $derived() syntax is a function-like expression.

The $derived() call is a syntax that closely resembles a function call, but derived itself is a keyword, not a function. You cannot alias it like const myDerived = $derived, which will throw a SyntaxError.

Blatantly plagiarised from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/import#syntax

@lucaesposto
Copy link

How about $derived.fromFunction or $derived.fromMethod, or something similar?

@Rich-Harris
Copy link
Member

good point @Not-Jayden — I've opened #10334 with a suggestion

trueadm added a commit that referenced this pull request Jan 31, 2024
* chore: add $derived.fn rune

* fix strange bug

* update types

* remove prev stuff

* regenerate types

* $derived.fn -> $derived.call

* docs

* regenerate types

* get rid of $$derived

* tighten up validation etc

* fix tests

---------

Co-authored-by: Rich Harris <rich.harris@vercel.com>
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.