Skip to content
This repository was archived by the owner on Feb 12, 2022. It is now read-only.

Add babelHelpers mock for fb-www #1354

Closed
wants to merge 2 commits into from

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Jan 25, 2018

Needed for our www class transform.

I verified extends works. However I get weird results with inheritance.

Input: https://gist.github.com/gaearon/f70a187badf877e3d29c49252471686d

Output without #1348: (crashes)

Invariant Violation: undefined
This is likely a bug in Prepack, not your code. Feel free to open an issue on GitHub.
    at invariant (/Users/gaearon/p/prepack/lib/invariant.js:20:15)
    at Functions.checkReactRootComponents (/Users/gaearon/p/prepack/lib/serializer/functions.js:204:35)

Output with #1348: https://gist.github.com/gaearon/3c60fce62a354b3571618704b51aeb2a

(note it uses undefined props variable).

@trueadm
Copy link
Contributor

trueadm commented Jan 25, 2018

If we change the inherits method to be:

    subClass.prototype = Object.create(superClass && superClass.prototype, {
      constructor: {
        value: subClass,
        enumerable: false,
        writable: true,
        configurable: true
      }
    });
    if (superClass)
      Object.setPrototypeOf
        ? Object.setPrototypeOf(subClass, superClass)
        : (subClass.__proto__ = superClass);

You no longer get broken output. Maybe we should use this version instead?

@gaearon
Copy link
Contributor Author

gaearon commented Jan 25, 2018

We could, but is it worth figuring out why the current version breaks?

@trueadm
Copy link
Contributor

trueadm commented Jan 25, 2018

@gaearon I'm not even sure how it works, maybe it relies on some special internal hacks/logic that I'm not aware of. See my inline comment.

Object.assign(subClass, superClass);
subClass.prototype = Object.create(superClass && superClass.prototype);
subClass.prototype.constructor = subClass;
subClass.__superConstructor__ = superClass;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does __superConstructor__ even do? Every other polyfill does a __proto__ assignment or setPrototypeOf here instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an FB specific thing, I don't actually remember what it does. Need to check.

Copy link
Contributor

@trueadm trueadm Jan 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you change that one line to __proto__, everything works as expected. but because we are setting it to be superClass, it tries to emit the function React.Component instead; which isn't right :/

@gaearon
Copy link
Contributor Author

gaearon commented Jan 25, 2018

Regardless of what the polyfill does or doesn't do, isn't Prepack emitting variable access for undeclared variables always a bug? I don't understand how valid JS code (app code + polyfill code), even if buggy, can result in broken JS output (undeclared variables).

@trueadm
Copy link
Contributor

trueadm commented Jan 25, 2018

@gaearon The function is missing the arguments props, context from React.Component. So that seems like a bug here (but that should be probably be made a separate issue rather than tagged to this one).

What I was trying to say was that we shouldn't be serializing the mock React.Component at all but the polyfill seems to be making it happen. If we change the polyfill to use React.Component as the prototype, Prepack correctly serializes the output out.

We should write some test cases for these bugs too.

@gaearon
Copy link
Contributor Author

gaearon commented Jan 25, 2018

The function is missing the arguments props, context from React.Component. So that seems like a bug here (but that should be probably be made a separate issue rather than tagged to this one).

Makes sense, thanks. Yeah I wasn't saying it's related to this issue in particular, I just wanted to point out this seems like a bug in Prepack regardless of this implementation.

Fixes lint error
Copy link

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trueadm is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants