-
Notifications
You must be signed in to change notification settings - Fork 148
[main] avoid getters when possible, and apply getters recursively #446
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
base: main
Are you sure you want to change the base?
Conversation
|
I feel like this is an aggressive optimization. The compiler produces more closure than before. |
|
Hmm.. I'm not sure about this one. I know I suggested this as a possibility. The problem is it changes expectation between code inlined and not inlined. Every time in the past I've done this we've gotten in trouble. To be fair you can consider ternaries and conditionals in JSX of this level of optimization but the downside is too common and too high if we are to say people can use those. Whereas this saying we shallowly wrap props makes sense because if someone passes an object from somewhere else it won't get this treatment. I think this needs more thought. |
|
I think the main problem this fix illustrates, is that objects that aren't supposed or expected to be reactive become reactive because of the getter. If the object were inlined, when not in need to be reactive, it will behave more "as expected", as in theory you should only expect changes from things that have been defined to change. I'm speaking from my experience, say you have <Component style={{ width: size.width, height: size.height }} />and size is a regular object that happens to be updated when the window resizes. If you happen to read the Oh well, kind of bad example, I understand this specific case could be reading from a store. OK, but what if I do this : <Component style={{ width, height }} />Then that illustrates what I meant, I think . But honestly, I didn't check to what this compiles. Point still stands, things becoming reactive from unreactive objects. |
It adds the optimization you suggested on discord, recorded here #390
It considers spreads and computed keys, such
{[store.key]: "ja"}, on which case it will keep using a getter. It also considers@once.It may be possible that we want this optimization in other places 🤔 , as when I search for
"get"similar code popups around. Can you please have a look? Thanks!I'm like 95% confident this won't break stuff, but honestly I'm not sure.
Initially the recursion was a bit messy, I have just simplified it.