Replace object getter with Object.defineProperty in TestRenderer#10473
Replace object getter with Object.defineProperty in TestRenderer#10473flarnie merged 4 commits intofacebook:masterfrom
Object.defineProperty in TestRenderer#10473Conversation
**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
|
We have tried to avoid transforms that potentially add a lot of code because it will lead to unexpectedly slow code and larger file sizes. I think this might be one right? I'd prefer to check in the changed code. |
|
Sure thing - will check in the changed code. We should come up with some other way of making sure this doesn't happen again - still have not figured out how I could have checked that this syntax was not supported. We could either add a lint or something on our side, or discover a new step for testing when syncing. |
|
Why would we need this transform or code-change? Getters for non-computed props are well-supported (IE9+). Edit: Sorry. I saw this PR before I saw your comment about Haste in the group chat. |
**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
flarnie
changed the title
Object.defineProperty in TestRenderer
| return wrapFiber(root.current.child); | ||
| }, | ||
| enumerable: true, | ||
| configurable: true, |
There was a problem hiding this comment.
I'm curious- why does this need to be enumerable or configurable?
There was a problem hiding this comment.
I was copying what the babel transform would do - seems like a reasonable default. They would both default to false otherwise.
Since it's not _root, it seems like it should be enumerable and configurable.
There was a problem hiding this comment.
enumerable perhaps but I don't think it should be configurable. (Both of these default to false by the way, which I think would also be reasonable in this case.) 😄 Not a big deal though just sharing my thoughts
There was a problem hiding this comment.
Pretty much every property we have is enumerable and configurable since that's the default for assignment and object literal construction. So why should this property be different? The reason configurable is useful is that it allows for patching, polyfilling and backward compatibility fixes. The downside is that it might allow for bad patching, polyfilling etc. :)
There was a problem hiding this comment.
It felt weird for a getter that might throw to be enumerable. I don't hold this opinion strongly though. 😄
**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
|
Updated things a bit because:
|
**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:**
5 participants
what is the change?:
Replaces an Object Getter 1 with
Object.defineProperty.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