Skip to content
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

Components using Array.map to construct children do not satisfy conditions for the transform #552

Open
alxclark opened this issue Apr 4, 2024 · 4 comments
Labels

Comments

@alxclark
Copy link

alxclark commented Apr 4, 2024

Using a simple component that maps onto an array, the transform does not see that the children are actually using signals and does not trigger the transform.

Example:

export function App({ name }) {
  const greetings = ["Hello", "Goodbye"]
  const children = greetings.map((greeting) => <div key={greeting}>{greeting} {name.value}</div>)

  return <div>{children}</div>;
}

I opened a draft PR adding a test showing the error case: https://github.com/preactjs/signals/pull/551/files#r1552520125

@alxclark
Copy link
Author

alxclark commented Apr 5, 2024

After digging into the babel plugin for a little while, it looks like the following happens when using a map:

  • We iterate over all MemberExpression
  • We find the signal access in Array.map
  • We attempt to set on the parent function scope that this function uses signals
  • We end up writing on the arrow function of the map instead of the component
  • We iterate over function calls, check if they both contain JSX and signals
  • Turns out the function is not marked as using signals since it was added to the map's arrow function scope.
  • Transform ignores our component.

A naive and not performant solution would be to recursively write to the parent until we hit the root of our program, which should hopefully be just a handful of levels deep.

There's probably a better way of solving this, I'd love some feedback from folks more familiar with babel plugins. 😄

@XantreDev
Copy link
Contributor

Yes, we can do it here, until reach parent

setOnFunctionScope(path, maybeUsesSignal, true);

@XantreDev
Copy link
Contributor

I can make a pr. It shouldn't impact performance

@alxclark
Copy link
Author

alxclark commented Apr 5, 2024

@XantreDev That would be awesome, I'm more than happy to review once you do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants