ryansolid / dom-expressions

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

fix(dom-expressions): handle expression in ref #317

Closed mdynnl closed 5 months ago

mdynnl commented 5 months ago

fixes https://github.com/solidjs/solid/issues/2137

these are still relevant here so didn't touch them

let refTarget;
const template8 = <div ref={refTarget} />;
const template10 = <div ref={refFactory()} />;

not sure why the check for call expression was there though.

SarguelUnda commented 5 months ago

Posting here to not flood the issue we came from.

As a general case I understand your point. I was today year old when I realised that props?.ref isn't lhs aswell.

Isn't there an argument to transform props?.ref into props.ref to get the optimisation of the lvalcond instead of the more general use function that your merge request propose ?

SarguelUnda commented 5 months ago

Moreover if props.ref is not à function your fix doesn't work either no ?

mdynnl commented 5 months ago

https://github.com/ryansolid/dom-expressions/blob/ac40fb85d27d119ee940d3f555e668ebaeaea22f/packages/babel-plugin-jsx-dom-expressions/test/__dom_fixtures__/attributeExpressions/code.js#L65

transforms to

https://github.com/ryansolid/dom-expressions/blob/ac40fb85d27d119ee940d3f555e668ebaeaea22f/packages/babel-plugin-jsx-dom-expressions/test/__dom_fixtures__/attributeExpressions/output.js#L170-L175

and because of this https://github.com/ryansolid/dom-expressions/blob/ac40fb85d27d119ee940d3f555e668ebaeaea22f/packages/babel-plugin-jsx-dom-expressions/src/dom/element.js#L498

it only works for a call expression,

but it should work for anything that isn't a literal, let variable, member expression.

ryansolid commented 5 months ago

Yeah I think you are right. I probably was fixing a specific issue and was looking too narrow. That being said the fact this change hasn't changed the output of any tests suggests to me are definitely missing some tests we should have.

ryansolid commented 5 months ago

Thank you for adding the tests. Much appreciated.