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

Support trailing $ name convention for stores (Observables) #6373

Closed
btakita opened this issue May 30, 2021 · 15 comments
Closed

Support trailing $ name convention for stores (Observables) #6373

btakita opened this issue May 30, 2021 · 15 comments

Comments

@btakita
Copy link
Contributor

btakita commented May 30, 2021

The RxJS community has adopted trailing $ for an observable name. Svelte supports leading $ is naming the value of the store (observable).

It strikes me that the trailing $ on the store would be more consistent with standard javascript naming practices, especially when working with large codebase having helper functions.

Svelte

<script>
import { writable } from 'svelte/store'
const amount = writable(0)
</script>
You have {$amount} comments.

RxJS

<script>
import { onMount, onDestroy } from 'svelte'
import { of } from 'rxjs'
const amount$ = of(0)
let amount,subscription
onMount(()=>subscription = amount$.subscribe(a=>amount = a))
onDestroy(()=>supscription.unsubscribe())
</script>
You have {amount} comments.

Yes, the svelte example has less code & is easier to read, however it can be improved by also supporting trailing $.

<script>
import { writable } from 'svelte/store'
const amount$ = writable(0)
</script>
You have {amount} comments.

Helper functions in javascript modules

Creating a function that takes a store or a value of a store as an argument, the programmer is faced with a quandary. What name should the value variable be?

Should it be amount or $amount? In normal javascript & with observables having the trailing $, the name of the value would be amount. With the value having the $ prefix, the name of the value is $amount. However, this does not work well when passing the value as a property of an object. Should all props also be named $amount? What about api calls? As some point, the name amount will be required to reperesent the value, which contradicts svelte's naming convention.

Solution

Support both the prefix $ for values and the suffix $ for stores. Both naming conventions can be supported by the svelte compiler.

<script>
import { writable } from 'svelte/store'
const amount$ = writable(0)
const name = writable('Mr Bean')
</script>
{$name} has {amount} beans.

This will fix naming collision by making the store have the $ suffix. The store is not passed data & if it is, would follow the convention set by RxJS. Also the additive effect of the $ suffix implies that the observable characteristic is wrapping the value, instead of infering from the store.

Current implementation of svelte

The $ suffix can be added to the store, but the $ prefix of the value is still required.

<script>
import { writable } from 'svelte/store'
const amount$ = writable(0)
const name = writable('Mr Bean')
</script>
{$name} has {$amount$} beans.

Adding support for the $ store suffix stripping off the $ would fix the naming issues of support functions on the value of the store.

@Conduitry
Copy link
Member

Interop with other ecosystems or no, this would be a breaking change.

@btakita
Copy link
Contributor Author

btakita commented May 30, 2021

I'm not aware of how this would be a breaking change but I'll take your word for it. Could you elaborate?

My primary concern is that the $ prefix for values is not ergonomic when working with application-wide data flow, apis, helper functions, & when refactoring from a reactive value to a store. The interop would also be affected, in the case of RxJS, but since observable support subscribe, support as almost there. The unsubscriber behavior is different, but that could be smoothed over, perhaps with a compiler hook or preprocessor? I invoked RxJS into this github issue to show there is precedent to the $ suffix. I know @Rich-Harris is interested in standards among different libraries from a tweet a couple of years ago. Perhaps people in the RxJS community can comment on the subtleties on why the $ suffix was chosen for observables.

An example that just came up, when refactoring a reactive variable to a shared store:

From:

<script>
let amount = 0
</script>
You have {amount} comments.

Currently, one has to also change the reading of the value to $amount:

<script>
import { amount } from './shared/amount'
</script>
You have {$amount} comments.

With the $ suffix convention, no changes are necessary to read the value:

<script>
import { amount$ } from './shared/amount$'
</script>
You have {amount} comments.

@dummdidumm
Copy link
Member

It's a breaking change because everyone using the foo$ syntax and using foo in the same file would have different behavior. Also I guess it's way harder to to write the logic for that because you substract, not append a character.

@btakita
Copy link
Contributor Author

btakita commented May 30, 2021

It's a breaking change because everyone using the foo$ syntax and using foo in the same file would have different behavior.

To your point there is a breaking change with the current compiler rule, where foo would be assumed to be a value of foo$ if foo$ exists in the file. I wonder how common this is.

Also I guess it's way harder to to write the logic for that because you substract, not append a character.

That's a good consideration. The compiler would need to do look for variables using something like variable.match(/([a-zA-Z_\$][a-zA-Z0-9_\$]*)\$/)[1] instead of basing the evaluation on existing variables beginning with $ using something like variable.match(/$([a-zA-Z_\$][a-zA-Z0-9_\$]*)/)[1].

So the difference would be not only the match, but what drives the match. For the $ prefix on the value, the value drives the match. For the $ suffix on the store, the store drives the match. Supporting both would mean the compiler would need to support both ways of matching. The suffix approach by itself seems to be about the same amount of difficulty, but supporting both would seem to add more difficulty.

Would you agree with that assessment?

I don't know if the ergonomics of the $ value prefix is a common complaint, but after working with Svelte for a while on large codebases, using the $ prefix in ts/js files is one of those slightly (pebble in a shoe) annoying anti-patterns that can pervade the entire codebase. Like a pebble in a shoe, it's such a seemingly small issue that one does not need to address it for short walks, but the issue grows as a hike gets longer.

I'm playing around with different naming conventions & patterns. If anybody has suggestions or wants to continue the discussion, I would appreciate it.

@Rich-Harris
Copy link
Member

Not gonna lie, this is a bit of a lightbulb moment. I just wish we'd had it two and a half years ago!

The more idiomatic RxJS interop is cool, but I'm much more persuaded by the refactoring argument. It's something I've run into on occasion, and it's a a legitimate criticism people have made of Svelte's reactivity model, wherein stores are perceived to be second-class citizens compared to component-local state (or props).

I'm also persuaded by the thought that foo$ communicates store-ness better than a foo that happens to be read in proximity to an occurrence of $foo.

Two points:

  • This is a breaking change, however unlikely it is that someone would declare both foo$ and foo in the same file (not a confusing break, since the compiler can disallow a foo declaration or import in a scope that already has foo$ defined; it would fail in a straightforward and communicable way rather than a cryptic one. But a break nonetheless). If we claim to respect semver then realistically we're talking about Svelte 4 here
  • One of Svelte's unwritten tenets is that we should provide one way to do things. I don't think we'd ever support both foo/$foo and foo$/foo, that would be hellaciously confusing and unnecessarily complex in terms of implementation.

So the question is 'is this change of sufficient value that it's worth the extreme disruption it would cause?' I suspect most people here would say 'no', but I also think there's at least a possibility that it is, insofar as it would fix one of Svelte's (arguably) most noticeable warts.


Having said all that, the main frustration I encounter when dealing with stores is the requirement that they be free variables rather than (e.g.) object properties. Some code from a project I'm currently working on:

export let runner;

const weight_idle = spring();
const weight_running = spring();
const weight_celebrating = spring();

$: weight_idle.set($runner.state === 'idle' ? 1 : 0);
$: weight_running.set($runner.state === 'running' ? 1 : 0);
$: weight_celebrating.set($runner.state === 'celebrating' ? 1 : 0);

Having three variables for each of those weights, rather than a single object or map with idle, running, celebrating etc properties, makes me feel grubby. I already know that I'm going to need to add a bunch more different weights; it would be great if I could do something like this instead:

export let runner$;

const weights = {};
states.forEach(state => weights[state + '$'] = spring());

$: states.forEach(state => {
  weights[state + '$'].set(runner.state === state ? 1 : 0); 
});

// later
$: angle = weights.idle * angles.idle + weights.running * angles.running;

Since the compiler can't realistically know which properties of weights are stores (especially if that object is imported), the detection would have to happen at runtime, and at a cost.

So if we're discussing the possibility (however remote!) of changing store syntax, I'd love for us to see if we can come up with an approach that makes it possible to solve this problem at the same time.

@Rich-Harris
Copy link
Member

@pngwn notes this related issue #2016

@pngwn
Copy link
Member

pngwn commented May 30, 2021

Conversation regarding object property store auto-subscriptions (sometimes dubbed 'contextual stores') should probably into #4079 as there is a bit more conversation there. I'll close #2016 but capture the example as it is a clear example.

@arxpoetica
Copy link
Member

FWIW, I have deliberately written foo$ in files because $foo is reserved for stores. I used the convention to show when a variable was a reference. Just a footnote of data for "have people done this?" At least one person. 😆

@btakita
Copy link
Contributor Author

btakita commented Jun 18, 2021

Thank you for the discussion. I have some good news to report. I have been rolling out the val$ naming convention & all of the naming conflicts around stores has been resolved, particularly in *.ts files. My projects tend to have the majority of the logic in stores & less logic in the components. I have also broken out Controller classes for logic-heavy templates, which includes functions, writable & derived stores. The naming conflicts resolved include:

  • Before: should a $ prefix be applied to local variables? After: the value variable name has no $ character
  • Before: props: should a $ prefix be used on the prop name? After: The prop name has no $ character
  • Before: In a sapper/svelte kit preload function, the $ prefix cannot be used for local variables due to the Svelte Compiler. After: No $ prefix necessary to distinguish between store & value.

The $val$ component values are also not too bad to work with either. It conceptually makes sense in a certain way, as the $ prefix & suffix cancel each other out, like a + & - cancel each other out in arithmetic. It is an extra character, but one could make the aesthetic argument that $val$ is balanced. The $ prefix can be thought of as a preprocessor operator on the store$ object.

With that in mind, I'm not too concerned about changing the syntax, as long as $ prefix & suffix ($val$) is not too much of an eyesore & burden for the programmer. I'm not opposed to changing the syntax either, but having gone through 2 migrations, I can see that it would be a sizable effort for large codebases. Less than 1 week in most cases, but still sizable. I think it's worth doing, but keeping the migration optional has it's merits.

To keep the value in the same form when refactoring from a Component variable to a store, perhaps another svelte preprocessor "op" and/or label could be added?

<script>
import { value$ } from './value$'
value: $value$
</script>

<input bind:value>

Another reason to not change the syntax is that sometimes it's useful to have an export val, val$, & $val$ used in the same component. I have used this pattern with a component with export val which also have a val$ store in the Controller or shared store.

<script lang=ts>
import { value$ } from './value$'
export value:string
value: $value$
</script>

<input bind:value>

@Rich-Harris Correct me if I'm wrong but, would the bottom block not work in svelte 3 as long as runner is seen as an invalidatable variable?

export let runner$;

const weights = {};
states.forEach(state => weights[state + '$'] = spring());

$: $runner$, states.forEach(state => {
  weights[state + '$'].set($runner$.state === state ? 1 : 0); 
});

// later
$: angle = weights.idle * angles.idle + weights.running * angles.running;

https://twitter.com/wycats/status/1380386140478271488

Yehuda Katz had an interesting insight into composing reactive cell primitives (i.e. svelte stores). I'm looking for ways to ergonomically compose svelte stores into objects as well. I have started to add the _ get/set prop on the stores that I use. I'm currently using writable$, readable$, & derived$.

const val$ = writable$('')
val$._ = 'new value`
console.info(val$._)

It would be interesting to compose svelte stores into objects. For my purposes, mainly in Controller logic, having one of the above defined stores assigned to a prop works great. There's no need for decorators & reading/writing values from stores can occur inline. A decorator could be used on the Class as syntax sugar for the get/set proxy property to the store well. I'm not particularly interested in using decorators & proxy get/set props at this time, but Yehuda, Ember, MobX, Angular, & others use decorators & proxy get/set props. In Svelte components, there are also cases when calling the get/set props is necessary, such as in functions or reading the store value without using $$invalidate.

@btakita
Copy link
Contributor Author

btakita commented Jun 23, 2021

@ryansolid has astute commentary on stores as well. He addresses the trailing $ under the "Compilation Shortcomings" section.

https://itnext.io/designing-solidjs-reactivity-75180a4c74b4

I like using the trailing $ indicator (as Ryan calls it) to identify store cells to differentiate between the store & the value. If Typescript provided a polymorphic way to reference the value, like the svelte compiler, then the indicator would be superfluous. Svelte does provide an opportunity to have SolidJS's state api via Readable. I have implemented something similar but rarely use this pattern. I prefer treating stores as a cell which have a get/set method, which I use _ to minimize the syntax burden of referencing the value of the store (i.e. "cell"). It's not as seamless as a svelte reactive variable, but it's not too bad in practice & some naming convention patterns can be utilized.

Nonetheless, Solidjs is impressive & I share many of Ryan's sentiments including the observation that the local component scopes tend to not have as much of the app logic in more complex systems. I find that I'm using stores in components significantly more than reactive variables. His performance concerns are also interesting, since large apps have many components, so having a way to "inline" the components on compilation would be beneficial.

@dummdidumm
Copy link
Member

Leaving this here for possible later use, if this is implemented.

This is how to get the TS types:

type SvelteStore<T> = { subscribe: (run: (value: T) => any, invalidate?: any) => any }
type StoreValue<T> = T extends SvelteStore<infer Value> ? Value : never;

type WithStoreValues<Object> = Object & {
  [Property in keyof Object as Property extends `${infer Key}$` ? Key : never]: StoreValue<Object[Property]>;
};

type Works = WithStoreValues<{a: true; b$: { subscribe: (run: (value: boolean) => any, invalidate?: any) => any } }>;

https://www.typescriptlang.org/play?#code/C4TwDgpgBAygbhANsCNgHsBOEA8AVAPigF4oBvKAZwFcAjSgY0wEtaIAuKACk2oDtOXOAENE1DlDwBKEkWF8QAGijM+IxMwAmwlAH5O8kDOJyFUAL4BYAFChIsDNgBqo8fiKk8UCAA8UfTUpYBGRUR1xVADMITCgXMQgiXTjXaE4+CARMAG4bGztoAHVmYAALNCwIePFKHAB5WgArCAZgDygG5taoADJyGygoAG0ABUx0SExQFT4oAGsIEHRIjqaW4ChhILGJmOnff0CoAAMAEjIomKgAaUXzU+OoZNuQKHTMmIBdTgrnVPq1q1RuNJqBPgRctZzJD8uAilg5kFSMUyr8qqlamRhJxgLwINkoLRTpwKDR6ExWBIePxBOpxJxaOh0IgIPJjKYlDN1FodBB9JsFOyBa9zBYIUA

@stale
Copy link

stale bot commented Dec 24, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Dec 24, 2021
@rmunn
Copy link
Contributor

rmunn commented Dec 24, 2021

Further activity to prevent stale-bot from closing a proposal on which no decision has yet been made.

@bradphelan
Copy link

Not sure this adds more to the conversation but I've run across the problem in my own toy lib

https://github.com/bradphelan/immer.loves.svelte

The simple example is

<script lang="ts">
 type Data {
    readonly a:string
    readonly b:string
 }
  
  // declare the root store
  let store:Writable<Data>

  // declare subStores as projections onto the root store
  let aStore:Writable<string> = subStore(store,s=>s.a)
  let bStore:Writable<string> = subStore(store,s=>s.b)
</script>

<input type="text" bind:Value={$aStore}/>
<input type="text" bind:Value={$bStore}/>

requires creating those pesky local variables aStore and bStore. Would certainly be nice to be able to write the projections inline in the markup.

<script lang="ts">
 type Data {
    readonly a:string
    readonly b:string
 }

  let store:Writable<Data>

</script>

<input type="text" bind:Value={subStore(store,s=>s.a)$}/>
<input type="text" bind:Value={subStore(store,s=>s.b)$}/>

@dummdidumm
Copy link
Member

Closing as won't do - Svelte 5 introduces runes, which replace stores in most occurences and allow for fine grained reactivity (which was one of the reasons for wanting nested subscriptions); as such we're not gonna change its subscription syntax.

@dummdidumm dummdidumm closed this as not planned Won't fix, can't repro, duplicate, stale Oct 30, 2024
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

No branches or pull requests

8 participants