Open razshare opened 1 year ago
The main point of the discussion I would say is this.
When reactive statements get out of control their content should be wrapped within a named function with named parameters (dependencies).
This gives the developer a name to memorize/recall while scrolling the code, give an explicit and obvious set of dependencies the statement will react to, which is the arguments of the function, and the parameters will shadow the original variables names, not permitting their modification within the function scope.
I like this proposal a lot. I also find reactive statement to be quite messy when overused.
Should we enforce a naming convention for reactive functions? onChangeCounter
is pretty good but it also sounds like an event handler. I checked the entire HTML of the component to see if it was used on an on:click
because at first I thought it was also an event handler. I think "reactive functions" should not be easily confused with "event handlers functions". Also, how would we call a function that has multiple dependencies? onChangeCounterAndAchievementCompletedAnd...()
.
I propose that the name of the function does not incorporate the dependencies, so instead of onChangeCounter(counter)
I suggest onChange(counter)
.
onChange
really feels like an event handler. Could we call it onEffect(counter)
similar to useEffect()
hook in React. I'm really not a fan of naming it onEffect
but I don't see anything else that wouldn't be confused with an event handler.
What are your thoughts on that?
I propose that the name of the function does not incorporate the dependencies
The dependencies shouldn't be enforced into the name, but they shouldn't be discouraged either imo.
One of the reasons being that having the code warped in a function like that you can now manually invoke the function without changing its dependencies.
For example I would be able to call onChangeCount
with a static value
// ...
onChangeCount(1)
// ...
Having the name of the dependency in the function names has generally been pretty useful.
I might be biased because I come from a java background and that's the naming conventions repositories follow there, but regardless if it's coming from a brainless oop world or not, it's still very handy I think.
how would we call a function that has multiple dependencies?
When it comes to this I agree it's not good practice.
But I also have to say that whenever I would have multiple dependencies my thought process would usually go as follows:
onChange{name of the new interface}
See how the first point and the last one are linked logically?
sounds like an event handler.
It's supposed to sound like an event.
But I won't pretend it's not easy to mix the 2 types of events up, because it is pretty easy.
What prefix would you use?
For example I would be able to call onChangeCount with a static value
This sounds trivial, but you would be surprised how useful it becomes when you're dealing with css animations and spring transitions.
Being able to call the function arbitrarily avoids a lot of code duplication.
Just imagine an animation based on a point (x,y)
$: onChangeXY($x,$y)
function onChangeXY(x,y){
// do some calculations
}
To your point (no pun intended) the name onChangeXY
is a very ugly name, even difficult to type maybe, but usually it comes natural to collapse that into:
$: onChangePoint($point)
function onChangePoint(point){
const { x, y } = point
}
You answered all of my questions perfectly! I didn't see it at first but you are right on every point :)
What prefix would you use?
With the onChangePoint
example I think it's better without a prefix. That being said, would it be good to add a $
suffix after the function name? Maybe something like this:
onChangeCount$(count)
onChangePoint$(point)
let count = 0;
$: onChangeCount$(count);
function onChangeCount$(count) {
// ...
}
It would indicate that the function is automatically called by a $
reactive statement. The downside is that it is exactly like how QwikJS does lazy-loading and it also adds extra syntax to remember.
I really liked this 😂:
- Give up on explaining
With the onChangePoint example I think it's better without a prefix. That being said, would it be good to add a $ suffix after the function name? Maybe something like this:
I'm not opposed to "$" at the end, even if it's not as clean, other frameworks use it, as you mentioned Qwik does and Angular uses that too for observables.
The only problem is that I think there are plans for Svelte 5 to adobt the "$" at the end of the store name (instead of at the beginning of the name, as it is right now), mainstreaming it with Qwik and Angular naming conventions.
Maybe we can wait for Svelte 5 to be released before adding "$" at the end in this recommendation.
This is very opinionated stuff, so in the meantime the more suggestions we get here the better.
What do you think of
$: newCount($count)
function newCount(count){}
Sounds good to me, let's wait for Svelte 5!
What do you think of
This looks great but I now that I think about it you're right, it's better onChangeX...()
. I think a prefix or suffix would be the solution but the name should stay like an event handler since it's pretty much one.
Ok, so far the proposed naming convention seems to be onChangeX
or onChangeX$
, where X
is an arbitrary name not enforced, but possible the names of the dependencies.
so for example
$: onChangePoint($point)
function onChangePoint(point){
// ...
}
or
$: onChangePoint$($point)
function onChangePoint$(point){
// ...
}
Whether or not we should use $
as a suffix will depend on Svelte 5 naming conventions.
What do you think of the positioning of the reactive statement and the function declaration?
Should they be near each other?
$: onChangeCount($count)
function onChangeCount(count){
// ...
}
$: onChangePoint($point)
function onChangePoint(point){
// ...
}
or some other style, say for example: all reactive statements at the top and functions at the bottom?
$: onChangeCount($count)
$: onChangePoint($point)
// some other code in between
function onChangeCount(count){
// ...
}
function onChangePoint(point){
// ...
}
I can see pros and cons for both.
The first style is a bit easier to navigate, but it would be harder to figure out what are all the reactive statements in the given component.
While the second style would hit you immediately with all the available reactive statements and their respective functions but it would be harder to navigate between reactive statements and functions, it could turn into a scrolling hell.
I think this positioning is easier to understand and scale:
$: onChangeCount($count)
function onChangeCount(count){
// ...
}
$: onChangePoint($point)
function onChangePoint(point){
// ...
}
Or this but without code between reactive statement and function declaration
// code before
$: onChangeCount($count)
$: onChangePoint($point)
function onChangeCount(count){
// ...
}
function onChangePoint(point){
// ...
}
// code after
I think we should open an issue for recommendation concerning the orders of a script block. Example:
<!-- Ideal -->
<script>
// Imports
// Props (export let ...)
// local variables (let ...)
// context variables (setting or getting from context API)
// functions
// reactive statements
// Other things I don't have in mind currently
</script>
With a recommendation well defined, we would be in a better position to decide what positioning approach is the best for explicit reactive statement.
I think we should open an issue for recommendation concerning the orders of a script block.
Yeah that sounds great.
Totally a beginner perspective, but I think I'd find it easier to read through all the reactive statements in one section and then have the functions together.
1) I agree that when you get too many reactive statements, using functions is cleaner to read. 2) I don't like the onChangeX naming. You should name your function based on it's business use case and not the code use. So not onChangeCounter(), but instead incrementAcheivements()
This is akin to the rule that your click handler should not be called clickHandler, it should be named something that makes sense to what the code is doing. It's great that counter is changing, but why and what are we doing with is what the name should indicate.
3) I agree with reactive $: directly above the function it's calling. It holds the context of the reader and allows the dev to make a great comment above the block.
4) Not a fan of the $ in the fn name. Svelte already has 3 distinct uses of $, we really don't want to throw in a 4th new usage. (reactive statements, stores and magic paths)
So not onChangeCounter(), but instead incrementAcheivements()
What happens if a function doesn't contain business logic?
Using that logic how would you modify this code
<script>
let counter = 0
let style = ''
$: if (counter > 5) {
style = 'color: red'
} else {
style = 'color: balck'
}
</script>
<button class="btn btn-primary" on:click={() => counter++}>
<span>Click me</span>
</button>
<span {style}>{counter}</span>
Would you call the function modifyColor
?
Modify
to what though?
Or would you call it toggleColorBetweenRedAndBlack
, in that case aren't we back here?
how would we call a function that has multiple dependencies? onChangeCounterAndAchievementCompletedAnd...().
I don't think it's a good idea, people will not always find the right name for their functions and will go astray.
These recommendations, I think, should provide clear ways of naming these things, so that given a project and a new developer joining it, the developer would recognize the pattern.
These names should also look familiar jumping from one project to another.
Things should be easy to memorize and generally speaking there should one way of naming things, in my opinion.
Imposing something so subjective as leaving it up to the business logic is the same as not imposing any rules at all.
If we're gonna define things we shouldn't define them for the business team or even the project manager, these should be things defined to help the developer, especially new developers, who most of the time will not even understand the full business logic behind a project.
You can always add your business-named function within the event itself, so this
<script>
$: onChangeCounter(counter)
function onChangeCounter(counter) {
incrementAchievements(counter)
}
</script>
In your example, the business logic is changing the font color, so I would
<script>
let counter = 0;
let style = '';
$: updateTextColor(counter);
function updateTextColor(counter) {
style = counter > 5 ? 'color:red': 'color:black';
}
</script>
<button class="btn btn-primary" on:click={() => counter++}>
<span>Click me</span>
</button>
<span {style}>{counter}</span>
I don't like the onChangeX naming. You should name your function based on it's business use case and not the code use. So not onChangeCounter(), but instead incrementAcheivements()
I like this a lot! I've never thought about this but it makes so much sense. I would much rather read a function name that tells me what it does than when it is triggered. Also, if we collocate the reactive statement above the function declaration, it's easy to see when it is triggered.
but instead incrementAcheivements()
I'm really not convinced by this, it leaves too much up to interpretation.
We should favor readability and hinting.
These functions can also be called outside of a reactive statement, with that in mind, why would a function called updateTextColor
take a counter: number
as a parameter?
This also introduces back dependency cycles because we're encouraging creating multiple functions that react to the same dependency, unless I'm missing something.
What happens when you declare 2 of these functions and they both modify the same data, whilst one of them is using await tick()
for example?
Having 1 single function that deals with a specific domain of dependencies means that you can also sync them using syntax sugar like await
.
I think writing these things should be almost like an instinct. We could even make some snippets/IDE extensions that maybe autocomplete "onchange" into this template.
Maybe I just can't see it.
We should ask more people what they think of this.
Totally a beginner perspective, but I think I'd find it easier to read through all the reactive statements in one section and then have the functions together.
We also need to sort this one out.
Tomorrow I'll be working on https://github.com/svelte-cig/svelte-common-recommendations/issues/2#issuecomment-1685307999 after that, either Sunday or the next week, we'll need to sort out a way to vote on these recommendations, using some sort of polling.
You should name your function based on it's business use case
Don't get me wrong, I agree with this, but not all code is business logic.
Changing the color of a label on the client has nothing to do with business logic.
As far as I'm concerned business logic is code that at least does some crud operation.\ Even then, opinions among many developers may change on what business logic is.
In which case we either define "business logic" properly and write that in big letters, or leave it up to interpretation.
I don't want this recommendation to introduce discussions between developers in a team about "what business logic is" about similar subjective topics.
Should we recommend to use named reactive statements in every situations or only when x amount of reactive statements are present in a component?
Currently, naming an ultra simple function based on it's business name seems pointless, but if we consider that named reactive statement will be used for complex use case it makes more sense.
Could we combine the two names options to get something like incrementAcheivementsOnChangeCounter()
or updateTextColorOnChangeCounter()
? It would make some pretty long function name, but does it really matter? If the function is complex, it's name should provide enough information to get a small idea of what it does.
Also, should a recommendation work for every edge cases?
What happens when you declare 2 of these functions and they both modify the same data, whilst one of them is using await tick() for example?
This seems like a pretty rare situation, should choose to recommend something that works for edge cases like this one? Maybe, in the documentation, we could provide a general recommendation with small modifications for rare edge cases?
What problem does this recommendation solve?
Whenever dealing with reactive statements I've seen people often inline their statements directly in the root of the script, sometimes even conditions are inlined.
I've never had the experience of reading and maintaining Svelte code I had never seen in a large project, but I've had the responsability to explain a large codebase to a junior dev fresh from university 3 years ago.
He did not have a great time reading stuff like this
https://github.com/svelte-cig/svelte-common-recommendations/blob/2f32acb6fd67f4515414e987fbc3f54d8793cb22/explicit-reactive-statements/AppNotOk.svelte#L15-L22
Now imagine that type of code in a large project, reactive statements upon statements upon more statements, some depending on each other.
Reactive statements are great, but should not be used alone, because they can get very hard to read and can lead to sneaky bugs.
As a matter of fact the code above does not even compile, because there's a cyclical dependency described by
achievementCompleted -> counter -> achievementCompleted
.Sure the language server will help you identify these things, but if I change the first statement like this
all of a sudden things become more difficult to understand
which one creates the cycle? counter? achievements?
You'll need to split the line in multiple lines against your formatter wishes and find out that way.
Combine that with ternary operations being very popular in reactive statements, and you've got yourself a full weekend of debugging.
Recommendation
My solution always comes down to declaring reactive statements in a more explicit manner.
That means
These are the only 3 golden rules of reactive statements.
Using the example from before, this is how the code looked like before
https://github.com/svelte-cig/svelte-common-recommendations/blob/2f32acb6fd67f4515414e987fbc3f54d8793cb22/explicit-reactive-statements/AppNotOk.svelte#L1-L42
And this is how it looks like after applying these rules.
https://github.com/svelte-cig/svelte-common-recommendations/blob/2f32acb6fd67f4515414e987fbc3f54d8793cb22/explicit-reactive-statements/App.svelte#L1-L47
Let's discuss.