Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete Stack reconciler #10517

Closed
wants to merge 13 commits into from
Closed

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 23, 2017

Follow up to #10511.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 23, 2017

(Edit: filed #10518 for this. Added FlowFixMe for existing issue for now.)

(Edit 2: rebased on master where this is fixed)

I almost fixed Flow after replacing ReactInstanceType with Fiber but this code puzzles me:

// TODO (bvaughn) Rename the findNodeHandle module to something more descriptive
// eg findInternalHostInstance. This will reduce the likelihood of someone
// accidentally deep-requiring this version.
function findNodeHandle(componentOrHandle: any): any {
  if (__DEV__) {
    // TODO: fix this unsafe cast to work with Fiber.
    var owner = ((ReactCurrentOwner.current: any): Fiber | null);
    if (owner !== null) {
      warning(
        owner._warnedAboutRefsInRender,
        '%s is accessing findNodeHandle inside its render(). ' +
          'render() should be a pure function of props and state. It should ' +
          'never access something that requires stale data from the previous ' +
          'render, such as refs. Move this logic to componentDidMount and ' +
          'componentDidUpdate instead.',
        getComponentName(owner) || 'A component',
      );

      owner._warnedAboutRefsInRender = true;
    }
  }

In my understanding if we actually go down that code path in Fiber, we'll throw because we can't define _warnedAboutRefsInRender on a Fiber (it's non-extensible). Equivalent code in findDOMNode makes sure to put it on the stateNode but that doesn't seem to be the case here.

So is it a bug? Does the warning unintentionally throw?

@@ -108,9 +108,40 @@
"version-check": "node ./scripts/tasks/version-check.js"
},
"jest": {
"projects": [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried keeping just one project but even the newest version of Jest (21 alpha) crashes with just one project so I had to inline the config back. cc @cpojer in case this is not known.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, it's a known issue you already filed jestjs/jest#4117 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, thanks. I remember a few issues were recently closed so I wasn't sure. Sorry I didn't check 😛

@cpojer
Copy link
Contributor

cpojer commented Aug 23, 2017

⚛️🎉

I'm stacking every fiber in my body in favor of this PR.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 24, 2017

I think this is ready for review.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 24, 2017

@NE-SmallTown
Copy link
Contributor

NE-SmallTown commented Aug 24, 2017

Hi, I want to know how to find the complete src code(because maybe I want to learn Stack reconciler) after we delete Stack reconciler?

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 24, 2017

You can always view old release tags on GitHub or check out old branches like 15-stable in git.

@NE-SmallTown
Copy link
Contributor

Thanks, I forget that. So will we update Stack reconciler on old branches like 15-stable in the future?

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 24, 2017

No, there are no plans to work on Stack. Fiber replaces it.

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 26, 2017

Rebased.

@gaearon
Copy link
Collaborator Author

gaearon commented Sep 24, 2017

Split into four parts.

Safe to land now:
#10794
#10796

Worth testing internally:
#10797
#10798

@gaearon gaearon closed this Sep 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants