Skip to content

Commit

Permalink
Replace object getter with Object.defineProperty in TestRenderer (#…
Browse files Browse the repository at this point in the history
…10473)

* Add babel transform for object getters

**what is the change?:**
We were not transforming object getters[1], and our new TestRenderer
uses one.[2]

[1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get
[2]: https://github.com/facebook/react/blob/master/src/renderers/testing/ReactTestRendererFiberEntry.js#L569

**why make this change?:**
Our internal build system was not supporting object getters.

**test plan:**
`yarn build && yarn test`
Also built and opened the 'packaging' fixture.

Honestly I'm not sure what else to test, this seems pretty low risk.

**issue:**
None opened yet

* Replace object getter with `Object.defineProperty` in TestRenderer

**what is the change?:**
Replaces an Object Getter [1] with `Object.defineProperty`.

[1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/get

**why make this change?:**
Our internal build system does not support object getters.

**test plan:**
We should do follow-up work to ensure that this doesn't come up again -
we can't commit code which uses object getters.

**issue:**
No issue opened yet

* Tweak the Object.defineProperty call in test renderer to make flow pass

**what is the change?:**
- switched from `Object.defineProperties` to `Object.defineProperty`
- set some undefined properties to get flow to pass

**why make this change?:**
- Flow doesn't seem to play nicely with `Object.defineProperty/ies`
  calls, specifically when you implement `get` and `set` methods.
  See facebook/flow#1336
- Switched from `properties` to `property` because it seems more
  conventional in our codebase to use `property`, and I'm only setting
  one anyway.

**test plan:**
`flow`

**issue:**
no issue

* More flow type tweaking

**what is the change?:**
Forced the typing of an argument to 'Object.defineProperty' to be
'Object' to avoid an issue with Flow.

**why make this change?:**
To get tests passing and flow passing.

**test plan:**
`flow && yarn test`

**issue:**
  • Loading branch information
flarnie authored Aug 17, 2017
1 parent 34092a0 commit 149860b
Showing 1 changed file with 20 additions and 7 deletions.
27 changes: 20 additions & 7 deletions src/renderers/testing/ReactTestRendererFiberEntry.js
Original file line number Diff line number Diff line change
Expand Up @@ -565,13 +565,9 @@ var ReactTestRendererFiber = {
invariant(root != null, 'something went wrong');
TestRenderer.updateContainer(element, root, null, null);

return {
get root() {
if (root === null || root.current.child === null) {
throw new Error("Can't access .root on unmounted test renderer");
}
return wrapFiber(root.current.child);
},
var entry = {
root: undefined, // makes flow happy
// we define a 'getter' for 'root' below using 'Object.defineProperty'
toJSON() {
if (root == null || root.current == null || container == null) {
return null;
Expand Down Expand Up @@ -611,6 +607,23 @@ var ReactTestRendererFiber = {
return TestRenderer.getPublicRootInstance(root);
},
};

Object.defineProperty(
entry,
'root',
({
configurable: true,
enumerable: true,
get: function() {
if (root === null || root.current.child === null) {
throw new Error("Can't access .root on unmounted test renderer");
}
return wrapFiber(root.current.child);
},
}: Object),
);

return entry;
},

/* eslint-disable camelcase */
Expand Down

0 comments on commit 149860b

Please sign in to comment.