Fix overly aggressive function hoisting (#3125) #4000
Conversation
There was a problem hiding this comment.
I think this is going the right direction! @Conduitry or @Rich-Harris should probably weigh in.
Do you think looking into the other aliasing forms is worth it? I believe so since we have so many truly hoistable functions even accounting for aliasing.
|
Is it possible to write a runtime test for this instead of a js sample test, which is a lot more brittle? |
| @@ -160,4 +160,5 @@ export interface Var { | |||
| hoistable?: boolean; | |||
| subscribable?: boolean; | |||
| is_reactive_dependency?: boolean; | |||
| aliased?: boolean; | |||
There was a problem hiding this comment.
What exactly does the aliased field mean? I see that it's another thing that can stop variables from being hoisted, but I don't understand what it's checking for.
There was a problem hiding this comment.
The idea was to have aliased mean whether the variable is passed around functions or given other names as to avoid hoisting functions/objects because they could've been modified through these other names.
We'd still have to track all forms of aliasing though.
There was a problem hiding this comment.
Tracking all forms of aliasing doesn't sound like it would be possible. Is this another situation where we just need to document how the compiler works - like how only certain types of assignments trigger reactivity, rather than embarking on a never-ending journey to catch more cases?
There was a problem hiding this comment.
Hm, why not? We wouldn't need perfect tracking, just knowing whether the variable escapes a single name -- the possibility it's used elsewhere. The other option would be to disable function hoisting (like currently for objects), since the way it stands functions that shouldn't be hoisted, are.
| @@ -1079,7 +1093,7 @@ export default class Component { | |||
| } else if (owner === instance_scope) { | |||
| const variable = var_lookup.get(name); | |||
|
|
|||
| if (variable.reassigned || variable.mutated) hoistable = false; | |||
| if (variable.reassigned || variable.mutated || variable.aliased) hoistable = false; | |||
There was a problem hiding this comment.
I don't think this is the right place for this. This is marking the function not hoistable if it references aliased variables, instead of marking the function not hoistable if itself is aliased.
ghost
mentioned this pull request
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Closing as the PR is severely outdated, there doesn't seem concensus on how to move forward and the code might change anyway with v5 |
Attempt to fix #3125
Before submitting the PR, please make sure you do the following
npm run lint!)Tests
npm testoryarn test)