Refactor effects.data into properties #1943
Conversation
There was a problem hiding this comment.
Nice, thanks for doing this. Please attend to the flow errors.
I would appreciate it if there are no formatting changes that are not absolutely required by our prettier rules.
This is probably also a great opportunity to get rid of the somewhat gratuitous abbreviations for the property names of Effects.
| this.canBeApplied = true; | ||
| this._id = effects_uid++; | ||
| } | ||
|
|
||
| // TODO: Make these into properties | ||
| data: [EvaluationResult, Generator, Bindings, PropertyBindings, CreatedObjects]; |
There was a problem hiding this comment.
We would like to declare the properties here.
src/evaluators/DoWhileStatement.js
Outdated
| @@ -79,8 +79,8 @@ export default function( | |||
| let result = realm.evaluateForFixpointEffects(iteration); | |||
| if (result !== undefined) { | |||
| let [outsideEffects, insideEffects, cond] = result; | |||
| let [rval] = outsideEffects.data; | |||
| let [, bodyGenerator] = insideEffects.data; | |||
| let { result: rval } = outsideEffects; | |||
There was a problem hiding this comment.
I'm all for consistency, but I do have to wonder if it would be more readable to just use the property directly, i.e.
let rval = outsideEffects.result;
| @@ -216,7 +216,7 @@ function ForBodyEvaluation( | |||
| invariant(result instanceof JoinedAbruptCompletions); | |||
| invariant(result.consequent instanceof ContinueCompletion || result.alternate instanceof ContinueCompletion); | |||
| joinedEffects = extractAndSavePossiblyNormalCompletion(ContinueCompletion, result); | |||
| result = joinedEffects.data[0]; | |||
| result = joinedEffects.result; | |||
There was a problem hiding this comment.
Here is an example where we are already inconsistent and it doesn't seem quite so bad.
src/methods/join.js
Outdated
| @@ -68,8 +68,14 @@ function joinArrayOfsMapEntries( | |||
| let n = Math.max((a1 && a1.length) || 0, (a2 && a2.length) || 0); | |||
| let result = []; | |||
| for (let i = 0; i < n; i++) { | |||
| let { $Key: key1, $Value: val1 } = (a1 && a1[i]) || { $Key: empty, $Value: empty }; | |||
| let { $Key: key2, $Value: val2 } = (a2 && a2[i]) || { $Key: empty, $Value: empty }; | |||
| let { $Key: key1, $Value: val1 } = (a1 && a1[i]) || { | |||
There was a problem hiding this comment.
Why did the formatting change?
There was a problem hiding this comment.
I had prettier-eslint turned off. I'll fix it.
src/evaluators/CallExpression.js
Outdated
| @@ -66,17 +66,29 @@ function callBothFunctionsAndJoinTheirEffects( | |||
| invariant(Value.isTypeCompatibleWith(func1.getType(), FunctionValue)); | |||
| invariant(Value.isTypeCompatibleWith(func2.getType(), FunctionValue)); | |||
|
|
|||
| let [compl1, gen1, bindings1, properties1, createdObj1] = realm.evaluateForEffects( | |||
| let { | |||
| result: compl1, | |||
There was a problem hiding this comment.
It will probably be quite nice to just stick with the property names of Effects.
There was a problem hiding this comment.
What would you say is more readable, to keep destructuring:
let {
result: result1,
generator: generator1,
modifiedBindings: modifiedBindings1,
modifiedProperties: modifiedProperties1,
createdObjects: createdObjects1,
} = realm.evaluateForEffects(...or to name the objects and call their properties directly ?
const e1 = realm.evaluateForEffects(...)
...
new Effects(e1.result, e1.generator, e1.modifiedBindings, e1.modifiedProperties, e1.createdObjects),
...There was a problem hiding this comment.
I went for the second option whenever possible, as it is a bit more terse.
pmrcunha
force-pushed
the
refactor-effects-properties
branch
from
48f2d10
to
c3ced14
Compare
pmrcunha
force-pushed
the
refactor-effects-properties
branch
from
c3ced14
to
52c7bd2
Compare
There was a problem hiding this comment.
@hermanventer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Release Note: none
Hi! I started trying to learn this codebase today and found this TODO from @cblappert in
realm.js:https://github.com/facebook/prepack/blob/aa700a2667f5c28754ae2dad2a64a3ad4191276a/src/realm.js#L88
Sounded like a good task to get into, so I went through the codebase and did the necessary replacements. If this wasn't the intent, please disregard this PR.