From 962e70ad52d5c63dd3faff47c62321c56a060b70 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 16 Aug 2017 11:36:50 -0700 Subject: [PATCH 1/4] 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 --- .babelrc | 3 ++- package.json | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/.babelrc b/.babelrc index f2c53291163d..b17cf7063178 100644 --- a/.babelrc +++ b/.babelrc @@ -20,6 +20,7 @@ ["transform-es2015-destructuring", { "loose": true }], ["transform-es2015-block-scoping", { "throwIfClosureRequired": true }], "transform-es3-member-expression-literals", - "transform-es3-property-literals" + "transform-es3-property-literals", + "transform-es5-property-mutators" ] } diff --git a/package.json b/package.json index 90511293b6af..14a14794aff6 100644 --- a/package.json +++ b/package.json @@ -30,6 +30,7 @@ "babel-plugin-transform-es2015-template-literals": "^6.5.2", "babel-plugin-transform-es3-member-expression-literals": "^6.5.0", "babel-plugin-transform-es3-property-literals": "^6.5.0", + "babel-plugin-transform-es5-property-mutators": "^6.24.1", "babel-plugin-transform-object-rest-spread": "^6.6.5", "babel-plugin-transform-react-jsx-source": "^6.8.0", "babel-preset-react": "^6.5.0", From f04a4dc4c593a2ab7b8c6a9411b231605faddc0a Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 16 Aug 2017 12:45:47 -0700 Subject: [PATCH 2/4] 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 --- .babelrc | 3 +-- package.json | 1 - .../testing/ReactTestRendererFiberEntry.js | 23 +++++++++++++------ 3 files changed, 17 insertions(+), 10 deletions(-) diff --git a/.babelrc b/.babelrc index b17cf7063178..f2c53291163d 100644 --- a/.babelrc +++ b/.babelrc @@ -20,7 +20,6 @@ ["transform-es2015-destructuring", { "loose": true }], ["transform-es2015-block-scoping", { "throwIfClosureRequired": true }], "transform-es3-member-expression-literals", - "transform-es3-property-literals", - "transform-es5-property-mutators" + "transform-es3-property-literals" ] } diff --git a/package.json b/package.json index 14a14794aff6..90511293b6af 100644 --- a/package.json +++ b/package.json @@ -30,7 +30,6 @@ "babel-plugin-transform-es2015-template-literals": "^6.5.2", "babel-plugin-transform-es3-member-expression-literals": "^6.5.0", "babel-plugin-transform-es3-property-literals": "^6.5.0", - "babel-plugin-transform-es5-property-mutators": "^6.24.1", "babel-plugin-transform-object-rest-spread": "^6.6.5", "babel-plugin-transform-react-jsx-source": "^6.8.0", "babel-preset-react": "^6.5.0", diff --git a/src/renderers/testing/ReactTestRendererFiberEntry.js b/src/renderers/testing/ReactTestRendererFiberEntry.js index bf0835afbb89..ece003fa4ddb 100644 --- a/src/renderers/testing/ReactTestRendererFiberEntry.js +++ b/src/renderers/testing/ReactTestRendererFiberEntry.js @@ -565,13 +565,7 @@ 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 = { toJSON() { if (root == null || root.current == null || container == null) { return null; @@ -611,6 +605,21 @@ var ReactTestRendererFiber = { return TestRenderer.getPublicRootInstance(root); }, }; + + Object.defineProperties(entry, { + root: { + 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); + }, + enumerable: true, + configurable: true, + }, + }); + + return entry; }, /* eslint-disable camelcase */ From f150345f8fa7a226836ee0a53fb9b23a9da3bce2 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 16 Aug 2017 16:34:29 -0700 Subject: [PATCH 3/4] 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 https://github.com/facebook/flow/issues/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 --- .../testing/ReactTestRendererFiberEntry.js | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/renderers/testing/ReactTestRendererFiberEntry.js b/src/renderers/testing/ReactTestRendererFiberEntry.js index ece003fa4ddb..88d9096cd57b 100644 --- a/src/renderers/testing/ReactTestRendererFiberEntry.js +++ b/src/renderers/testing/ReactTestRendererFiberEntry.js @@ -566,6 +566,8 @@ var ReactTestRendererFiber = { TestRenderer.updateContainer(element, root, null, null); var entry = { + root: undefined, // make flow happy; + // we set this below with Object.defineProperty toJSON() { if (root == null || root.current == null || container == null) { return null; @@ -606,17 +608,16 @@ var ReactTestRendererFiber = { }, }; - Object.defineProperties(entry, { - root: { - 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); - }, - enumerable: true, - configurable: true, + 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); }, + value: undefined, // make flow happy }); return entry; From 1811c3cde7d6273010a7160edf2674716da61ed6 Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 16 Aug 2017 18:17:07 -0700 Subject: [PATCH 4/4] 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:** --- .../testing/ReactTestRendererFiberEntry.js | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/renderers/testing/ReactTestRendererFiberEntry.js b/src/renderers/testing/ReactTestRendererFiberEntry.js index 88d9096cd57b..fff2175023e5 100644 --- a/src/renderers/testing/ReactTestRendererFiberEntry.js +++ b/src/renderers/testing/ReactTestRendererFiberEntry.js @@ -566,8 +566,8 @@ var ReactTestRendererFiber = { TestRenderer.updateContainer(element, root, null, null); var entry = { - root: undefined, // make flow happy; - // we set this below with Object.defineProperty + 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; @@ -608,17 +608,20 @@ var ReactTestRendererFiber = { }, }; - 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); - }, - value: undefined, // make flow happy - }); + 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; },