refactor(reactivity): unSignal #64
Conversation
|
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to fd3e2e4 in 1 minute and 33 seconds. Click for details.
- Reviewed
67lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. packages/unuse-reactivity/src/unSignal/index.ts:93
- Draft comment:
The peek method now temporarily disables subscription tracking and calls this.get(), which can trigger updates (e.g. through updateSignal and propagation) instead of simply returning state.value. Verify that this behavioral change was intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold50%The comment is asking the PR author to verify if a behavioral change was intended, which violates the rule against asking for confirmation of intention. However, it does provide a specific observation about the change in behavior, which could be useful for the author to consider. Despite this, the primary focus is on verification, which is not allowed.
2. packages/unuse-reactivity/src/unSignal/index.ts:75
- Draft comment:
Changing from arrow functions to method shorthand alters the binding of 'this'. Ensure that these methods (get, peek, set, update) are always invoked as object methods; otherwise, if they are destructured, 'this' might be undefined. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_tggEOOaqvdJ1CrX5
You can customize 
| } | ||
| }, | ||
| update(updater) { | ||
| this.set(updater(state.value)); | ||
| return state.value; |
There was a problem hiding this comment.
The update method now returns state.value, which deviates from the interface (expected to return void). Confirm if returning the new value is the desired behavior.
| return state.value; |
| peek() { | ||
| const prev = setCurrentSub(undefined); | ||
| const val = this.get(); | ||
| setCurrentSub(prev); |
There was a problem hiding this comment.
How is this different with previous solution?
There was a problem hiding this comment.
How is this different with previous solution?
There was a problem hiding this comment.
Doesn't state.value achieve the same goal?
There was a problem hiding this comment.
Yes, because this code does not run. I can't say how critical it is)) It takes time to understand the algorithm more deeply
There was a problem hiding this comment.
OK, I believe the key difference is https://github.com/un-ts/unuse/pull/64/files#diff-91b09a82c667732d70fec03508ee214b2fc202edd47a86c5252aa594f4a6f353R79?
|
Warning Rate limit exceeded@teleskop150750 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| } | ||
| }, | ||
| update(updater) { | ||
| this.set(updater(state.value)); | ||
| return state.value; |
There was a problem hiding this comment.
It you change the peek implementation, update should be changed at the same time.
| return state.value; | |
| return this.peek(); |
There was a problem hiding this comment.
Why do we need changes here?
There was a problem hiding this comment.
If state.value is always source of truth, why peek() needs change?
| } | ||
| }, | ||
| update(updater) { | ||
| this.set(updater(state.value)); |
There was a problem hiding this comment.
Should we also change this line?
| this.set(updater(state.value)); | |
| this.set(updater(this.peek())); |
There was a problem hiding this comment.
I understand your logic. For some reason it seems to me that the insert operation can be done without it. ))
My motivation was to save the source code to get the value
There was a problem hiding this comment.
logic for computed and signal has become uniform
There was a problem hiding this comment.
We don't have to uniform them if they don't need to, and we should be consistent in unSignal itself.
| setter(updater(state.value)); | ||
| peek() { | ||
| const prev = setCurrentSub(undefined); | ||
| const val = this.get(); |
There was a problem hiding this comment.
nitpick: I'm not a fan of this, that's why I also declare function variables inside the function body first and then "export/expose" in the return statement what's needed.
Instead we could use the code-style like in every other composable like this:
Details
export function unSignal<T>(initialValue?: T): UnSignal<T> {
const state: UnSignalState<T> = {
previousValue: initialValue as T,
value: initialValue as T,
subs: undefined,
subsTail: undefined,
flags: 1 satisfies ReactiveFlags.Mutable,
};
const get: UnSignal<T>['get'] = () => {
const value = state.value;
if (
state.flags & (16 satisfies ReactiveFlags.Dirty) &&
updateSignal(state, value)
) {
const subs = state.subs;
if (subs !== undefined) {
shallowPropagate(subs);
}
}
if (REACTIVITY_STATE.activeSub !== undefined) {
link(state, REACTIVITY_STATE.activeSub);
}
return value;
};
const peek: UnSignal<T>['peek'] = () => {
const prev = setCurrentSub(undefined);
const val = get();
setCurrentSub(prev);
return val;
};
const set: UnSignal<T>['set'] = (newValue) => {
if (state.value !== (state.value = newValue)) {
state.flags = 17 as ReactiveFlags.Mutable | ReactiveFlags.Dirty;
const subs = state.subs;
if (subs !== undefined) {
propagate(subs);
if (!REACTIVITY_STATE.batchDepth) {
flush();
}
}
}
};
const update: UnSignal<T>['update'] = (updater) => {
set(updater(state.value));
return state.value;
};
return {
[UN_SIGNAL]: true,
get,
peek,
set,
update,
};
}This resolves at the same time my other complain about code-style.
There was a problem hiding this comment.
I'd prefer TypeScript type inferring instead of declaring types everywhere explicitly, too verbose to write. 😂
There was a problem hiding this comment.
This is a special kind of composable structure, and the typedefs are there to bind them strictly to the exposing results. Otherwise they are function instead of const fnVar
Important
Refactor
unSignalfunction inindex.tsby inliningsetterlogic intosetmethod and updatingpeekto prevent side effects.unSignalfunction inindex.ts:setterfunction logic directly intosetmethod, removing separatesetterfunction.peekmethod to usesetCurrentSubto prevent side effects duringgetcall.This description was created by
for fd3e2e4. You can customize this summary. It will automatically update as commits are pushed.