search

solidjs / signals

136 stars 3 forks source link

API Review #2

Closed mihar-22 closed 1 year ago

mihar-22 commented 1 year ago

In this issue, I'm documenting the API differences between the current primitives in the wip directory and what's publicly documented for Solid and internally available.

Current API

✅ Valid

⚠️ API Differences

❓ Discussion

Base Signals Options

There's some minor differences in the current base options:

Not Implemented

The following APIs have not been implemented yet:

Integrations

The following are integration helpers. We could consider exposing these and also having helpers for popular frameworks such as React, Preact, Vue, and Svelte. Pretty sure Preact Signals was toying with something like that too.

Out Of Scope

Assuming anything advanced related to rendering or suspense is out of scope for this library (at least for now):

Breaking Change Proposals

Here's some things I'd like to propose that would be breaking changes but nothing that a simple find and replace can't resolve:

❓ change equals option to dirty

It seems odd to me that we provide an equality check function to signals. I think it's more intuitive that you're validating whether there was change which in my experience has always been a dirty check.

❓ change naming from owner to scope

Ownership in this library not only owns the lifecycle of children but handles things like context and error handling. This means where we decide to declare signals changes the runtime behaviour. This is essentially lexcial scoping which is a well known and understood term in programming and JS-land. Overall I think the term "scope" fits better than "owner" here.

❓ listener to observer

Listeners might be confusing as native DOM event handlers are generally called listeners (i.e., addEventListener). I also think most modern reactivity libraries (especially signals-based) refer to this type of identity as an observer, and we already refer to it in the new core internally as observers.

❓ rename mapArray to createComputedKeyedMap

We deal with computations in this library and most APIs here are called createX where X is some computation-type. The name mapArray doesn't make it clear what type of computation it's creating or what it does at all. I think we consider renaming this function to better align with the library and also be clear in it's intentions.

❓ rename indexArray to createComputedMap

Same as above.

❓ batch and on

I'd put these up for review as well based on the same arguments above.

Final Notes

Overall we look to be in good shape. Most things are just up for discussion and then going forward with implementing the missing APIs. While this discussion takes place I can start porting over tests from both Maverick and Solid. Eager to hear more thoughts!

ryansolid commented 1 year ago

Thanks for the writeup.

Current API

✅ Valid

  • createSignal
  • untrack
  • getOwner
  • runWithOwner
  • onError

Funny enough getOwner and runWithOwner have been causing some havoc in recent issues in Solid's repo. I want to revisit the design here but I wouldn't worry about it. I think removal of batching alone might simplify those issues.

⚠️ API Differences

  • createRoot Accepts a detached owner in Solid as a second argument - trivial to implement.
  • createEffect In Solid the second argument is the initial value - currently accepts options and no initial value yet. Secondly, we don't track the previous result and pass it back in right now but that's trivial to implement.
  • onDispose: This is called onCleanup in Solid.

Base Signals Options

There's some minor differences in the current base options:

  • id needs to be changed to name
  • dirty needs to change to equals

As you said these are all trivial to implement and probably should be. The one thing I think about now is that initialValue should be part of the options object but I don't know if it is worth a trivial break like that. We will go with Solid API for now worry about that later.

  • createMemo We batch by default now so what place does this have? We should probably rename it to createComputed or consider having memo as a lazy alternative?
  • tick This is new since we batch by default. The name and design is up for discussion.

createMemo show be the lazy thing. Assuming this still based on Milo's work the idea is that it only evaluates when asked like reactively's computed or what not. We are going to stay with the createMemo name even if it differs from most other reactive implementations.

tick or flush... tick makes sense if you await it like Svelte does. Otherwise if it just runs things synchronously I think flush or flushSync is fine using the React naming conventions.

Not Implemented

The following APIs have not been implemented yet:

  • createContext: we have ownership tracking now so this is trivial to implement.

Agreed.

  • useContext: we have the lookup function in wip/core.ts - trivial to implement.

Agreed.

  • batch: deprecate? I guess there could be an argument that queueMicrotask isn't reliable and we rather provide true control on when the batch is executed.

Deprecate. It isn't important enough and this gets messy giving that control.

  • on: run user given deps and untrack callback - trivial to implement.

Agreed. I dislike this primitive and wish I designed it differently. Mostly the extra code with the array iteration is unnecessary convenience that adds code and is pointless. We can make the call when we get here. This super low priority primitive.

Sure, I'm not sure what the memory efficient means here. Only comment not sure you have it but Solid detects if you use index or not to skip creating extra Signals. I imagine given Mavericks performance in benchmarks you do the same.

Same as above.

  • createComputed: our current createMemo implementation is exactly this - we need to decide on what role createComputed and createMemo will play here.

I want to deprecate this. createComputed in Solid today is like an eager Effect.. it's confusing an awkward to have. I think we might need another primitive to fill a gap.. one that merges, or what I was calling a writableMemo... but later concern.

Yes.

Sure interested to see this one closer. I benchmarked a bunch of things. It could have been the overhead of Solid's own signals but our current version was faster than anything I produced using Fabio's memory reduction tricks.

Integrations

The following are integration helpers. We could consider exposing these and also having helpers for popular frameworks such as React, Preact, Vue, and Svelte. Pretty sure Preact Signals was toying with something like that too.

Only comment here is our from isn't lazily subscribed which is awkward for Rx people... I figured once our createMemo is lazy that might be easier.

Out Of Scope

Assuming anything advanced related to rendering or suspense is out of scope for this library (at least for now):

For the most part I agree.. I think it might be hard to not include the async guts to createResource in here. Similarly the scheduling mechanisms behind renderEffect will need to be present here. We actually need to make a breaking change to renderEffect the way we are heading. Don't worry about this now. But I agree on the rest.

Breaking Change Proposals

Here's some things I'd like to propose that would be breaking changes but nothing that a simple find and replace can't resolve:

❓ change equals option to dirty

It seems odd to me that we provide an equality check function to signals. I think it's more intuitive that you're validating whether there was change which in my experience has always been a dirty check.

❓ change naming from owner to scope

Ownership in this library not only owns the lifecycle of children but handles things like context and error handling. This means where we decide to declare signals changes the runtime behaviour. This is essentially lexcial scoping which is a well known and understood term in programming and JS-land. Overall I think the term "scope" fits better than "owner" here.

❓ listener to observer

Listeners might be confusing as native DOM event handlers are generally called listeners (i.e., addEventListener). I also think most modern reactivity libraries (especially signals-based) refer to this type of identity as an observer, and we already refer to it in the new core internally as observers.

Naming convention for MobX was where equals comes from. I still like equals. I'm less interested in these sort of breaking changes to be fair. I do think the exposure of the Owner and Listener APIs need some looking at. Listener is awkward.. we do need to think about docs.

❓ rename mapArray to createComputedKeyedMap

We deal with computations in this library and most APIs here are called createX where X is some computation-type. The name mapArray doesn't make it clear what type of computation it's creating or what it does at all. I think we consider renaming this function to better align with the library and also be clear in it's intentions.

❓ rename indexArray to createComputedMap

Same as above.

Yeah.. I don't know if I like the names specifically proposed, but I can agree on the direction. For and Key isn't the connection most people make (even though it preferentially keyed). We can iterate on this.

❓ batch and on

I'd put these up for review as well based on the same arguments above.

I kinda wish these both would go away.. Someone suggested we call on, watch.. seems alright. I don't know. I'm going to lean towards less breaking changes where possible generally unless it involves gutting something.

mihar-22 commented 1 year ago

Thanks for the quick response @ryansolid!

Quick note that the only reason I'm trying to push for smaller breaking changes is because we won't get this opportunity again so best to discuss/implement now. I think consistent naming and API interfaces are really important to get right in a primitive library like this. I also think keeping docs and variety dev backgrounds in mind that could come in the future is important too.

The one thing I think about now is that initialValue should be part of the options object but I don't know if it is worth a trivial break like that. We will go with Solid API for now worry about that later.

I'd argue it's probably best we resolve this now and be done with it, unlikely we'll come back. An options object is going to give us that flexibility to introduce future options, and it aligns with our other APIs.

On that note, I'd probably recommend for consistency that createRoot also accepts an options object for the detached owner - this way it can also accept base options such as name for debugging.

createMemo show be the lazy thing. Assuming this still based on Milo's work the idea is that it only evaluates when asked like reactively's computed or what not. We are going to stay with the createMemo name even if it differs from most other reactive implementations.

👍

tick or flush... tick makes sense if you await it like Svelte does. Otherwise if it just runs things synchronously I think flush or flushSync is fine using the React naming conventions.

flushSync sounds like a better name to me too.

Sure, I'm not sure what the memory efficient means here. Only comment not sure you have it but Solid detects if you use index or not to skip creating extra Signals. I imagine given Mavericks performance in benchmarks you do the same.

By memory efficient I mean that we don't create functions under the hood. We initialize a Compuation instance and simply call read/write operations on it write.call(computation). We also have a root owner for the map so we can quickly dispose of the entire map in one swoop. Similar explanation for indexArray and createSelector.

Naming convention for MobX was where equals comes from. I still like equals. I'm less interested in these sort of breaking changes to be fair. I do think the exposure of the Owner and Listener APIs need some looking at. Listener is awkward.. we do need to think about docs.

I'm not too invested in equals vs. dirty (it's whatever) - probably preferential thing for most but no need to create a breaking change for it.

I think owner vs. scope might feel like bikeshedding but I really thing the definition of owner doesn't align with the responsibility, it's kind of misleading for the reasons I mentioned. I think it'll make our lives documenting and explaining "scopes" and scope trees much easier than "owners". The word owner also leads to strange sounding functions such as runWithOwner which I'd imagine which could be runInScope (example). I think it's important we give this one some thought.

The listener and observer is tricky because we probably shouldn't refer to something internally different to how we expose/document it publicly.

Yeah.. I don't know if I like the names specifically proposed, but I can agree on the direction. For and Key isn't the connection most people make (even though it preferentially keyed). We can iterate on this.

Ye the names I suggested kind of suck but glad we agree on the direction. We can discuss them more when we get a PR up for it.

Summary

Here's summary of the proposed changes so far:

❓

The only thing I'd say that's up for discussion is the owner/scope naming stuff. I know you want to keep just as-is. Maybe I can investigate in Solid Discord what devs think? If you're absolutely against it well I guess we can just move on. I will note I don't think it's something we can easily revisit.

mihar-22 commented 1 year ago

1.0 Compat

Leaving a note here based on a discussion with Ryan around being backwards compatible. Most of these changes are trivial enough to offer a 1.0 compat export. I think we can provide all the same exports as before, provide an implementation where possible or no-op if no equivalent. On top of that we can log helpful warnings and migration messages to smooth the process.

As an example, it could be aliased in most bundlers from this-package to this-package/v1 or something - not sure on the import specifier or export path.

After some thought this might be something we provide higher up in the Solid core package. I'll just leave this as food for thought for now.

mihar-22 commented 1 year ago

I think we've covered everything here now so closing it out.