solidjs / signals

MIT License
144 stars 4 forks source link

Refactor + Fast Context #10

Closed mihar-22 closed 8 months ago

mihar-22 commented 10 months ago

👋

This is a reimplementation of #5 because the source had been drastically changed. I wanted to simply implement the new fast context system but there was a bunch of issues I needed to tend to first:

Repo changes:

  1. There was duplicate code between bubble-reactivity and the reactivity stuff in the src root. I simply merged them together into the root. The project was quite hard to follow so I slightly moved a few things around.

  2. I added verbatimModuleSyntax to tsconfig.json so typed imports and exports are checked.

  3. I added a simple prettier plugin for sorting imports just to keep it cleaner. I also re-formatted all files.

  4. Specified all required exports for the package in src/index.ts as a bunch of stuff was missing.

Bug fixes:

  1. Signal setters were no longer accepting a function (e.g., (currentValue: T) => T) which was breaking some tests.

  2. There was a bunch of invalid and brokens tests that I removed/fixed.

  3. Owner disposal method was updated to match Maverick as I had found there was owners/computations not being GC'd due to zombie pointers.

Features:

  1. Error handling is now handled linearly without stopping and walking up the context tree. The array of error handlers are copied over from parent to child. I also moved the handleError function as a method on to the Owner class. Tests were updated.

  2. New context API was added which includes createContext, getContext, setContext, and hasContext. This API does not require walking up the tree anymore as the object is copied over from parent to child. If a new value is set, the context object is shallow cloned to make sure child values do not creep up to parents, otherwise all context values would be a singleton. Important to note it throws an error if an owner doesn't exist on call or a value was not provided either as a default or via setContext. Tests were included.

  3. I've written code in Maverick to handle appending an owner after creation to another owner (i.e., merging the owner tree). It's only ~150 bytes. I haven't ported this over yet but let me know if this sounds interesting. It came up for me when working with Web Components as the lifecycle and connection is async.

lxsmnsyc commented 10 months ago

Okay here's probably 2 new arguments on how I feel about the lack of support for undefined value in Contexts.

  1. We can potentially support undefined, if the defaultValue is intended that way, by checking the arguments length in createContext. There we can add a placeholder value (maybe a symbol) or optionally declare the defaultValue property if there's 0 arguments to defaultValue. If defaultValue contains that symbol or if the context doesn't have the defaultValue property, we can throw the error. We can retain the in check in hasContext and we get to keep support for undefined values.
  2. Drop support for defaultValue and force users to leverage hasContext to decide whether they should provide a defaultValue or not. Now this one is hot because this is highly conflicting with the currently existing Context API in the core repo.
mihar-22 commented 10 months ago

I got a sense of how you might want to support it but not really why. I'm struggling to understand why do all that seemingly magic stuff to support it. Not setting defaultValue doesn't clearly imply an intent of undefined. It might just mean no default value.

I don't feel there's a good argument to be made to support undefined. I mean we definitely can but that means getContext needs to have a return type of T | undefined and every usage has to check for undefined, or as we know in a large codebase it will just be littered with getContext(context)!. I don't see it working out well as the codebase grows.

The context doesn't have to exist which hasContext helps with, and a lack of value can be represented by null. Outside of those cases, undefined would generally be unintentional. Am I missing something?

lxsmnsyc commented 10 months ago

@mihar-22 The only concern is the user's intent to actually set undefined as defaultValue. Unlike in React, Solid allows optional arguments on createContext, and I believe some intentionally leaves it empty.

Although personally I find it weird that we allow everything except undefined (reminds me of this weird behavior in Solid where JS can throw everything, but Solid just decides to ignore undefined).

Which is why I offered two arguments, either allow supporting undefined or drop the concept of defaultValue.

mihar-22 commented 10 months ago

Understood! I'm not totally sure about removing defaultValue? It's pretty neat as you can easily create a fallback value and at the same time type the context. What if you want to provide a symbol or some kind of singleton across different computation roots. It does seem like a good to have.

lxsmnsyc commented 10 months ago

@mihar-22 By removing defaultValue we are pushing the responsibility to the user by implementing it on their own. Now, they are free to either throw their own error or return a default value.

function useSomeData() {
  if (hasContext(SomeDataContext)) {
    return useContext(SomeDataContext);
  }
  // here, I can throw an error or return a default value
  return MY_DEFAULT_DATA;
}
mihar-22 commented 10 months ago

Ye I was thinking you can do that too but it felt a little cleaner on creation? Tough, I'm in the middle and don't mind. I do find defaultValue at times confusing but that's mainly when it's not used at all like in React.

I'll side with general sentiment here and leave it open incase anyone else wants to chime in. Happy to change if needed.

ryansolid commented 8 months ago

Thanks for the PR. I somehow missed this while focusing on SolidStart. But this is back to where I want to be working.

ryansolid commented 8 months ago

LGTM.. most important thing to me is getting to some sort of baseline. There is a lot of things I want to mess around with.