Various fixes/changes and code changes to React compiler #1348
Conversation
There was a problem hiding this comment.
Can you explain some more about what the "this assignments" mechanism is try to detect and what the heuristic is here?
I wonder if you're going to be missing out on a lot of special cases here. It might be better to just run the evaluator and detect the patterns that you're concerned with in the evaluator.
src/react/utils.js
Outdated
| export function getThisAssignments(functionBody: BabelNodeBlockStatement): Set<string> { | ||
| let assignments = new Set(); | ||
| traverse( | ||
| t.file(t.program([t.functionDeclaration(t.identifier("dummy"), [], functionBody)])), |
There was a problem hiding this comment.
Why do you need to wrap this? We have other places where we traverse function bodies without
Also, this is suuuuper slow. So let's a add a todo about fixing this and unify with other things that traverses and caches.
There was a problem hiding this comment.
You can't traverse any other way, we do the same thing everywhere else.
src/react/utils.js
Outdated
| t.file(t.program([t.functionDeclaration(t.identifier("dummy"), [], functionBody)])), | ||
| { | ||
| "ThisExpression|Identifier": function(path) { | ||
| if (t.isThisExpression(path.node) || path.node.name === "this") { |
There was a problem hiding this comment.
Is there any time that a ThisExpression doesn't have name === "this"?
There was a problem hiding this comment.
ThisExpression doesn't have a name. This checks if the Identifier has a name of this (it can happen now and then, so this checks against it).
src/react/utils.js
Outdated
|
|
||
| if (parentPath && t.isMemberExpression(parentNode) && parentPath.parentPath) { | ||
| let topNode = parentPath.parentPath.node; | ||
| if (t.isAssignmentExpression(topNode) && topNode.operator === "=") { |
There was a problem hiding this comment.
There are other operators like += that are also assignment. Do they not count?
There was a problem hiding this comment.
@sebmarkbage You'd need to create the assignment to a value before incrementing right?
There was a problem hiding this comment.
Technically you don't need to, but I guess it's most likely that you would.
|
@sebmarkbage I can evaluate the constructor, but I was concerned about side-effects in the constructor. Until we start to push people away from side-effectful behaviour in the constructor, I'm not sure we can evaluate it with confidence. If we were to call the constructor of a component twice like we did other lifecycle events for async, I'm sure we'd see just as many issues. |
trueadm
force-pushed
the
react-simple-class-component-fixes
branch
from
c676de6 to
653e916
Compare
|
Related issue facebook/react/issues/12089 |
There was a problem hiding this comment.
Ok, accepting to unblock but I think we should see how far we can get with evaluating the constructor as a pure function since we're trying to enforce that anyway. Async relies on it.
Note that assignments can be hidden behind any call if it leaks.
function helper(obj) {
obj.foo = 123;
}
class Foo extends Component {
constructor() {
super();
helper(this);
}
}So even without assignments in the constructor body, you're not safe.
It is very easy to leak the entire this object so it can still have many unknown values.
But if you're evaluating it as a pure function, you can detect if it leaked or how it was mutated.
There was a problem hiding this comment.
@trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: Needed for our www class transform. I verified `extends` works. However I get weird results with inheritance. Input: https://gist.github.com/gaearon/f70a187badf877e3d29c49252471686d Output without #1348: (crashes) ``` Invariant Violation: undefined This is likely a bug in Prepack, not your code. Feel free to open an issue on GitHub. at invariant (/Users/gaearon/p/prepack/lib/invariant.js:20:15) at Functions.checkReactRootComponents (/Users/gaearon/p/prepack/lib/serializer/functions.js:204:35) ``` Output with #1348: https://gist.github.com/gaearon/3c60fce62a354b3571618704b51aeb2a (note it uses undefined `props` variable). Closes #1354 Reviewed By: trueadm Differential Revision: D6818619 Pulled By: gaearon fbshipit-source-id: 3afbb30ea157e4ce0e595313afeea011a5ea2e0c
Various fixes and changes:
mapOverArrayValue- >forEachArrayValuebytecodeReact output type for future useResidualReactElements.js->ResidualReactElementSerializer.js