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
No reaction? #4
Comments
Hey @mindplay-dk! I was able to quickly fix all of it except the Let me know if you find out what's wrong :) https://codesandbox.io/s/reactive-dom-maverick-forked-5z0pt3?file=/src/index.jsx |
Oh, whoops. 😁 How comes this appeared to be valid with the TS types though? |
Oh gosh I'm tired today. 😅 |
This is curious - it's issuing way too many updates... it looks like every property in every instance receives an update? Line 89: https://codesandbox.io/s/reactive-dom-maverick-forked-15t8q7?file=/src/reactive-dom.js There may be something about contexts/roots that I'm not understanding. 🤔
I think my confusion here stems from the fact that If you want to be really explicit, I would suggest the more verbose I haven't seen any framework doing exactly this, but honestly, this would be my preferred format:
This makes all the client code completely explicit, and it accurately reflects what these functions do. I don't like the React hooks convention of naming the getter The I also don't know why the API itself uses Just my Anyhow, I will probably just wrap these functions and re-export. |
Okay, so I actually tried the As for my little Sinuous clone, I've added some more log statements to the first version here: https://codesandbox.io/s/reactive-dom-maverick-njvhsw?file=/src/reactive-dom.js It looks like the problem is nested effects, which appear to all fire at the same time? If you try pressing the "+" button on one of the counters, you can see every effect in every parent effect firing along with it. So I don't know how this was intended to work, but this example previously worked with S.js and dipole as well, so there's something substantially different about how this works, I guess? I've never fully grasped these libraries beyond using them, tbh, so I can't really help. Do you plan to build a UI library based on this? Feel free to steal anything you can use from my crap. 😄 |
BREAKING CHANGE: Library authors can decide how to re-export bindings from this package. The `$` prefix was used a little inconsistently so it's best to drop it completely. see #4#issuecomment-1171106602
I decided to go ahead with this recommendation and drop
I made some important core fixes a little while ago so now that demo you showed me works perfectly: https://codesandbox.io/s/reactive-dom-maverick-forked-8zxrn5?file=/package.json
This is exactly what I'm going for and I share the love/hate here too. I don't particularly like
Agreed but who really wants to be writing Thanks heaps for sharing all this feedback! |
Honestly, I go back and forth on this. But imagine you have 50 lines of JSX and somewhere in the middle you see this:
How quickly can you parse this and spot the bug? In many cases, a variable named While, yes, some reactive frameworks will actually detect that And sure, TS might catch these, but the issue is human readability. Now picture this amid 50 lines of JSX:
This would immediately stand out to most people, I think? Anything named The getter is a getter, so some kind of convention is required at least. Even if it's this, that's still better:
At least something about this variable is indicating it's not actually the price. So maybe this convention is a nice middle ground?
Again though, what you take the Honestly most people will find it easier to parse the word "get" than a symbol like From my point of view, it becomes somewhat a question of whether you're optimizing for reading or writing - personally, I'd rather spend more time writing, if that makes code more readable, but I also understand that not everyone feels the way I do. (Not saying I would hate the Either way, thank you for engaging in this discussion! I'm really glad to have run into someone who seems to share so many of my ideas and opinions. 😄🙏 |
Hmm, there is still a problem with my toy example - it appears to drop all prior effects when I create a new effect in the same context. Here's a simplified example - everything looks fine at first, but as soon as you add a new counter, all the other counters become unresponsive. I backported this example to the previous version based on So it just looks like a difference in behavior, whether intentional or not. I've had this example working with both Again, don't feel obligated to look into this - it's just a toy project, I have no plans to publish a library. 😄 |
It seems in JS user-land the To be more precise, the
I see. I thought the most sane thing to do is dispose of nested computations (here) when it's re-evaluated. Maybe it's not 🤔 I'll think about this and come back to it. I'm pretty sure it's why the old counters are dying, and the new are reactive until another one is added. |
Sorry for the late response, I somehow missed the notifications for your replies. 😅 I upgraded to the latest Just for clarification, did you change anything in my sandbox? Or you just forked it so you could upgrade the dependency? (I only upgraded the dependency in my own sandbox, and everything appears to work.) |
No problem @mindplay-dk! I'm pretty sure I only updated Maverick. Feel free to check my fork to see if there's any differences :) |
How? I wish CSB had a source control interface. 😅 Either way, I think this issue is resolved. |
Don't feel obligated to respond to this at all. 😄
But I had this thing based on another reactive library:
https://codesandbox.io/s/reactive-dom-lifecycle-zhzw7
A basic, working Sinuous clone using
dipole
.Well, your library looks awesome, so of course I had to try to port from
dipole
to that:https://codesandbox.io/s/reactive-dom-maverick-njvhsw
Essentially, there's no effects happening anywhere, and I have no clue why. 🤷♂️
As said, don't feel obligated to respond - this is most likely not due to any issue with your library, but just my lacking understanding of how it works. I would have posted this in Discussions if it were enabled. 😄
The text was updated successfully, but these errors were encountered: