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

[core] Enable innerRef on Backdrop, List, MenuList and Paper #13722

Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Nov 28, 2018

Enables innerRef on Backdrop, List, MenuList and Paper. These are the components where we currently are attaching refs for findDOMNode.

This is step 1 in #13676 (comment) and a prerequisite for #13676.

patch-package is only a temporary workaround until reactjs/react-docgen#311 gets merged. If it doesn't get merged before v4 I would suggest we use a fork instead of patching transpiled files.

PS: I would hold off for now on forwarding refs for all components. I generally follow the argument that it's breaking component abstraction and we can always add forwarding refs later on if the use case makes sense. I would however also argue that forwarding refs on components that are clearly meant to render a DOM node (e.g. a Button or List component) is not an issue as long we are defining the interface as basic as possible. For example defining that the ref on Button returns a button HTML element will likely block us if we decide to use a div in the future. However saying that the ref is a generic HTML element is not bad at all in my opinion. Since react-dom does not support all use cases yet (e.g. focus or passive event listeners) we still need access to the actual DOM.

This  enabled by using withStyles innerRef in conjunction with
forwardRef.
This was only needed for forwardRef but since displayName
support is not really ready yet we don't need it  anyway
@eps1lon eps1lon added the core label Nov 28, 2018
@eps1lon eps1lon added this to the v4 milestone Nov 28, 2018
}
});

Backdrop.displayName = 'Backdrop';
Copy link
Member Author

Choose a reason for hiding this comment

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

See facebook/react#14319 for full explanation.

package.json Outdated Show resolved Hide resolved
@eps1lon eps1lon changed the base branch from next to master January 14, 2019 16:51
@eps1lon eps1lon modified the milestones: v4, v3.9.0 Jan 14, 2019
@eps1lon eps1lon changed the title [core] Add forwardRef to components for which we need refs internally [core] Enable innerRef on Backdrop, List, MenuList and Paper Jan 14, 2019
@oliviertassinari
Copy link
Member

It's a naive question. Do we need the forwardRef complexity if React ships reactjs/rfcs#97?

@eps1lon
Copy link
Member Author

eps1lon commented Jan 14, 2019

It's an incomplete and open RFC. I don't know what your timeline is but if we want to be strict mode compatible in v4 then we not only have to delay indeterminately until the RFC is settled. We also have to wait for it to be implemented, released and use that version as a peer dependency. Those are quite a bit of unknowns.

I'm not sure what you mean by complexity. The only complexity I can see right now is waiting for reactjs/react-docgen#311 to be released.

I don't want to merge this right now. I'd rather wait for the docgen feature to be released than introduce patch-package.

@eps1lon eps1lon added the on hold There is a blocker, we need to wait label Jan 14, 2019
@eps1lon eps1lon removed the on hold There is a blocker, we need to wait label Jan 29, 2019
@eps1lon eps1lon force-pushed the feat/core/forward-refs-in-function-components branch from f5b94ed to 1984ae7 Compare January 29, 2019 10:57
@eps1lon
Copy link
Member Author

eps1lon commented Jan 31, 2019

With the release of react-docgen@3 patch-package is no longer required. This PR is green from my point.

@oliviertassinari Are you ok with merging this PR or do you want to wait for the fragment ref implementation?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 31, 2019

@eps1lon I don't have a strong feeling about each outcome.

  1. I doubt a lot of people use the strict mode. I wish we had the Fragment ref.
  2. I guess we can revert once we have the Fragment ref.

@eps1lon
Copy link
Member Author

eps1lon commented Jan 31, 2019

  1. I doubt a lot of people use the strict mode. I wish we had the Fragment ref.

Don't forget ConcurrentMode or createRoot which enfore strict mode.

@eps1lon eps1lon modified the milestones: v3.9.0, v4 Feb 1, 2019
@eps1lon eps1lon changed the base branch from master to next February 1, 2019 09:50
@eps1lon eps1lon merged commit bb08918 into mui:next Feb 1, 2019
@eps1lon eps1lon deleted the feat/core/forward-refs-in-function-components branch February 1, 2019 14:24
eps1lon added a commit to eps1lon/material-ui that referenced this pull request Feb 5, 2019
eps1lon added a commit to eps1lon/material-ui that referenced this pull request Feb 5, 2019
eps1lon added a commit that referenced this pull request Feb 5, 2019
eps1lon added a commit to eps1lon/material-ui that referenced this pull request Feb 5, 2019
@zannager zannager added the core Infrastructure work going on behind the scenes label Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants