Support for defineEnumerableProperties #12703
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions. |
facebook-github-bot
added
GH Review: review-needed
CLA Signed
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
facebook-github-bot
added
GH Review: accepted
Import Started
|
@ericvicenti has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
This looks pretty safe and well-documented so I'm gonna push this through. But ideally I think we should share the implementation on with babel-helpers. cc @bestander |
|
@ericvicenti I agree, sharing the babel implementation of the helpers just make sense, and could prevent future issues for other use cases. Here is a similar case for an earlier rn version #4844. |
|
I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification. |
facebook-github-bot
added
Import Failed
and removed
Import Started
|
I find it very odd that such a minor addition would cause build failure. Anything I can do to move this along @ericvicenti? |
|
I am not familiar with how this works, I take it the pull request was rejected? |
Summary: **Motivation** detailed in #12702. **Test plan** 1) running [a piece of code](mobxjs/mobx#839 (comment)) that utilizes the helper without any modifications on the react native package results an error being thrown.  2) updating the list of helpers available by adding the definition of `defineEnumerableProperties` as provided by babel [babel/babel/packages/babel-helpers/src/helpers.js](https://github.com/babel/babel/blob/master/packages/babel-helpers/src/helpers.js#L275-L285) results no errors.  Closes #12703 Differential Revision: D4658120 Pulled By: ericvicenti fbshipit-source-id: 914aed4d313b3cc4f7ab99049d05d0aef269a3be
|
Nope, this was landed! I had to retry it because there was a flaky test failure that prevented it from landing the first time |
Summary: **Motivation** detailed in facebook#12702. **Test plan** 1) running [a piece of code](mobxjs/mobx#839 (comment)) that utilizes the helper without any modifications on the react native package results an error being thrown.  2) updating the list of helpers available by adding the definition of `defineEnumerableProperties` as provided by babel [babel/babel/packages/babel-helpers/src/helpers.js](https://github.com/babel/babel/blob/master/packages/babel-helpers/src/helpers.js#L275-L285) results no errors.  Closes facebook#12703 Differential Revision: D4658120 Pulled By: ericvicenti fbshipit-source-id: 914aed4d313b3cc4f7ab99049d05d0aef269a3be
Motivation
detailed in #12702.
Test plan
defineEnumerablePropertiesas provided by babel babel/babel/packages/babel-helpers/src/helpers.js results no errors.