search

ryansolid / dom-expressions

A Fine-Grained Runtime for Performant DOM Rendering
MIT License
863 stars 125 forks source link

only assign ref to a variable #332

Closed mdynnl closed 5 days ago

mdynnl commented 1 month ago

This fixes a bug where we generate ref assignment props.ref = or anything that is assignable (left value) which was also wrongly followed it in dom-expressions client.js (by me).

mdynnl commented 1 month ago

After fixing the workflow, I see there are some usages that will be broken by this PR.

https://github.com/ryansolid/dom-expressions/blob/45d6ad28f919160d6a16b2924385745718aa0c7c/packages/dom-expressions/test/dom/events.spec.jsx#L8-L12

There's also a question for class components this.ref although it doesn't work well 

let a = new A()

<a.render />
createComponent(a.render, {}) // `this` won't be passed so `this.ref` doesn't make sense

Anything related to this @trusktr?

mdynnl commented 1 month ago

Or could only fix dom-expressions client for now?

ryansolid commented 1 month ago

Well original thinking was that props.ref if provided would always get transformed into a function. So then we could always do a function check to make the decision. Which suggests that the fix for #316 may have not been correct. Which is why I see #333 reverting it..

However, I'm gathering that doesn' fix the original issue https://github.com/solidjs/solid/issues/2136. So you are attempting to do this by having the compiler be smarter about when it tries to assign stuff vs making it a function?

I guess the problem with the original code approach is it basically assumed there were no harm in assigning the ref element to props that was never getting it passed in anyway. Since there was no consumer on the otherside who cares. And our heuristics are insufficient to determine all the other cases.

My gut is we just merge #333 to revert the change since it appears the original fix broke stuff for the reporter anyway. And then we try to find better heuristics.

trusktr commented 1 week ago

As far as this stuff, if createComponent(a.render, {}) is the generated code, then yeah I think it would be convenient for it to be called with this as the instance, f.e. createComponent(a.render.bind(a), {}), which I think is reasonable because its not possible to write <a.render() /> syntactically. A workaround is render = () => {} in the class.