Skip to content

Delete Stack reconciler#10517

Closed
gaearon wants to merge 13 commits into
react:masterfrom
gaearon:remove-dom-stack
Closed

Delete Stack reconciler#10517
gaearon wants to merge 13 commits into
react:masterfrom
gaearon:remove-dom-stack

Conversation

@gaearon

@gaearon gaearon commented Aug 23, 2017

Copy link
Copy Markdown
Collaborator

Follow up to #10511.

@gaearon

gaearon commented Aug 23, 2017

Copy link
Copy Markdown
Collaborator Author

(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?

Comment thread package.json
"version-check": "node ./scripts/tasks/version-check.js"
},
"jest": {
"projects": [

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

cpojer commented Aug 23, 2017

Copy link
Copy Markdown
Contributor

⚛️🎉

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

@gaearon

gaearon commented Aug 24, 2017

Copy link
Copy Markdown
Collaborator Author

I think this is ready for review.

@gaearon

gaearon commented Aug 24, 2017

Copy link
Copy Markdown
Collaborator Author

@NE-SmallTown

NE-SmallTown commented Aug 24, 2017

Copy link
Copy Markdown
Contributor

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

gaearon commented Aug 24, 2017

Copy link
Copy Markdown
Collaborator Author

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

@NE-SmallTown

Copy link
Copy Markdown
Contributor

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

@gaearon

gaearon commented Aug 24, 2017

Copy link
Copy Markdown
Collaborator Author

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

@gaearon

gaearon commented Aug 26, 2017

Copy link
Copy Markdown
Collaborator Author

Rebased.

@gaearon

gaearon commented Sep 24, 2017

Copy link
Copy Markdown
Collaborator Author

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