Contexts used in components seem to be an anti-pattern

[janechu]

Some components use context as a means of passing information down to related components that are expected to be there.

According to React this is an anti-pattern: https://reactjs.org/docs/context.html

This would require a breaking change to be made in the following components:

• AutoSuggest
• Listbox
• ListboxItem
• Slider
• SliderTrackItem

There are alternatives presented in the React context documentation that would follow more closely with the principles React has established for passing information down to components.

Once it is determined which (if any) of these components should be updated to reflect the accepted React patterns, new tasks should be created for each component with a referral to this issue.

[chrisdholt]

@scomea can you investigate based on Jane's recommendation and consider what changes need to take place in the above components to better align with react patterns?

[scomea]

Where is the anti-pattern argument? We are using context as intended as far as I can tell.

[janechu]

From the linked documentation provided by React:

Context is designed to share data that can be considered “global” for a tree of React components, such as the current authenticated user, theme, or preferred language.

Context is primarily used when some data needs to be accessible by many components at different nesting levels. Apply it sparingly because it makes component reuse more difficult.

[M]oving more complexity higher in the tree makes those higher-level components more complicated and forces the lower-level components to be more flexible than you may want.

As far as I can tell, some of the components I listed pass logic or properties down that are specifically supposed to be used by a child related to that component. There are some different patterns listed in the document such as using readerProps and the React version of slots which are named children.

[scomea]

Context is used to expose data about the parent to all of it's children (ie. global for that tree) or to expose global functions. I want to double check to ensure how we're using it does not trigger extra renders (something I read made me wonder) but I don't see the problem otherwise.

[janechu]

So, I would say what we're really talking about is a scoping issue.

It's the same reason I suggested that the name in context changed to "@microsoft/fast-components-react-msft/*", because of potential conflicts.

If there are better patterns available, such as renderProps, we should use those instead of injecting component specific logic into the context.

The examples given by React illustrate this point (as to what should be considered global):

Common examples where using context might be simpler than the alternatives include managing the current locale, theme, or a data cache

I don't know that this is super pressing, but I do believe we should tackle it for our next upcoming major version.

[chrisdholt]

closed by #3517