-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
chore: unified expression metadata #12644
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small things, otherwise 👍
Another step towards the more comprehensive compile-time analysis of each block dependencies that will unblock #12511.
Currently, various nodes need to know whether the expressions they contain are dynamic (i.e. reference state, or could reference state) and/or contain call expressions (which may mean they need to be wrapped in a derived, or a separate effect or whatever). This happens in a slightly ad hoc and messy fashion.
This PR neatens it up, and also adds a
dependencies
set to each expression metadata object, which should soon enable more detailed analysis. The goal is to be able to optimise cases like this......where it's frivolous to wrap the array members in signals.
There is a blocker to doing that currently: we would need to call
context.visit(node.expression, ...)
inside theEachBlock
visitor. That's not possible, because the analysis-phase visitor merging prevents us from callingcontext.visit
. I've been pissed off at this for a long time, and this is the straw that broke the camel's back — it's time to overhaul the analysis phase and have a single visitor per node type. Going to work on that as soon as this is merged.Before submitting the PR, please make sure you do the following
feat:
,fix:
,chore:
, ordocs:
.Tests and linting
pnpm test
and lint the project withpnpm lint