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

WIP: Allow statify to be coerced to primitive #8

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

amygrinn
Copy link
Contributor

You mentioned in the comments that it would be cool if statify allowed direct access to its BehaviorSubject's value.

So far, this allows direct access to strings and numbers by coercion. Typescript allows this through intersection types.

I'm not quite sure how to proxy [Symbol.iterator] yet to allow for direct access to array properties, but I think it's possible. I added some notes in the test cases.

@amygrinn
Copy link
Contributor Author

amygrinn commented Dec 16, 2020

OK pretty good. I like this feature.

I added some technical debt: the coreProxy returned for proxify is wayyy different than the one for statify and it no longer makes sense to share the same functionality. I think we should break up coreProxy into individual traps and have the proxy generation actually occur in the proxify, statify functions.

This feature, along with directly setting values on statify, will allow for some pretty cool behaviors:

const state = statify({ a: 0 });
state.a.subscribe((a) => console.log('a:', a);
state.a++;
state.a += 3;

// a: 0
// a: 1
// a: 4

@amygrinn
Copy link
Contributor Author

If you'd like, we can even add an immutable array function library to allow shit like:

const state = statify({ a: ['Y', 'M', 'C' ]});
state.a.push('A');
console.log(state.a.join(''));

// YMCA

@kosich
Copy link
Owner

kosich commented Dec 17, 2020

Hey, Tyler! Welcome back! 🙂 🙌

This is a tough one! I've been wanting the [toPrimitive] thing for long, and I love the setting part: stf.count += 1 and especially the pushing part: stf.jobs.push(…)!

But lately, I had some doubts:

  • it has ambiguity with proxify: stf.name + '!' is different from prx.name + '!' — which might confuse users (I'm not sure it's TS detectable)
  • it's not clear what should be returned by stf.name.length. proxify surely returns an Observable, with statify — it's not clear. The same goes with methods, like toString() and push() — currently they are read aimed and return Observables.
  • and all the container-type structures, like arrays, — it's another world of confusion: is stf.jobs[0] an Observable of all first elements of an array or the current value?

Generally, when calling .subscribe or .value — it's clear when you fold the Observable, with toPrimitive — it's not as clear as I'd like (this is IMO, surely)

I don't really know how to mix this feature with proxify well (speaking API-wise, no clue how it would be implementation-wise). I'm dubious. What do you think about all this?

P.S.: and I really like the toJSON trap and the iterator thing is quuuite interesting!

@amygrinn
Copy link
Contributor Author

amygrinn commented Dec 17, 2020

I absolutely agree with you on most points and was actually thinking that this functionality could be a separate function from statify, maybe even a different package altogether, with only the direct access and direct setting ability. Partly to disambiguate from proxify and partly because there's no need to pollute the consumer's namespace with properties like value, _value, or next. The only properties we truly need to expose are subscribe, pipe, forEach and toPromise (maybe getValue would be convenient).

I could see many people particularly being frustrated by value and _value not being allowed:

const state = statify({ creditScore: { description: 'Excellent', value: 750 }});

state.creditScore.value.subscribe(() => console.log('Your credit score has changed')); // <-- error 

RE array properties: arr.length in statify should still return a statify object, or whatever we call it, that can be coerced to a number: +arr.length or arr.length.valueOf(). Same for arr[0], arr.filter(), etc...

RE properties like toString and valueOf: You see how I handled the date object valueOf, I was actually already thinking about opening that up so that every property returns the toString() or valueOf() function in the [Symbol.toPrimitive] trap:

[Symbol.toPrimitive]: () => readValue()[Symbol.toPrimitive],

That would allow consumers to define custom coercion behavior:

const state = statify({ a: { valueOf() { return 5; } });
console.log(+state.a); // 5;

RE folding: Coercing the statify object to a primitive does not fold the actual observable, it just retrieves a value from the current state:

const state = statify({ a: { b: 3 }});
const subState = state.a.b;
console.log(+subState); // 3
// Despite coercion, the substate still retains observable properties
subState.subscribe(() => console.log('Subscribed!')); // Subscribed!

@kosich
Copy link
Owner

kosich commented Dec 31, 2020

Hey, Tyler! Really sorry for not responding this long. The end of the year is a bit tense... 🙂
Happy New Year, btw! 🎄

Each time I return to this PR I get mixed thoughts: on one hand, I like this simplification of handling primitives. On the other hand, I'm afraid that under-the-hood conversion from statify to proxify will confuse users, e.g. state.salary.toString() — is a proxify, so primitives won't work in this case (I think, same goes with state.name.length). Maybe we'll need some intermediate class for this, that is not nextable (writable), but can provide access to the current value?
And I misused the "fold" term in my comment: I think I meant this statify->proxify conversion.

On the second another hand (yep, this situation is partially confusing due to three hands involved 😅) you've raised another very important question of accessing value fields. Which I don't know how to deal with atm.

There, just wanted to clarify this before the year ends 🙂
I'll definitely get to this with some fresh thoughts in the new year 😅

HNY! 🎊

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.

2 participants