Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Change context provider to only allow one child #278

Conversation

ConorGriffin37
Copy link
Member

@ConorGriffin37 ConorGriffin37 commented Jun 24, 2020

We are having an issue writing Rhodium tests due to RoactGamepad.Focusable.Frame always being called Focusable and not the actual given name. For example, if you have two frames created like this:

return Roact.createElement("Frame", {
	Size = UDim2.new(1, 0, 1, 0),
}, {
	LeftFrame = Roact.createEelement(RoactGamepad.Focusable.Frame),

	RightFrame = Roact.createEelement(RoactGamepad.Focusable.Frame),
})

Both these frames will be called Focusable instead of LeftFrame and RightFrame which can make traversing the tree to rhodium test things a problem.

Focusable creates the frame in the following way:

return Roact.createElement(FocusContext.Provider, {
	value = self.focusNode,
}, {
	Focusable = Roact.createElement(innerComponent, innerProps)
})

I can not think of a way to fix this in the RoactGamepad library without Roact changes and it seems like it is important that components like this can be created without causing the naming of created components to be unexpected.

There are two different ways I see we could fix this in Roact, either switching to only allowing Providers to have oneChild or allowing a render function to be passed to Providers instead of children like is done with Consumers. In this PR I decided to present the oneChild solution since this seems like a smaller API change but I am open to either solution or other suggestions.

Checklist before submitting:

  • Added entry to CHANGELOG.md
  • Added/updated relevant tests
  • Added/updated documentation

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.188% when pulling fd50d86 on ConorGriffin37:changeProviderToAllowNamePreservation into 60197ac on Roblox:master.

@LPGhatguy
Copy link
Contributor

This would be a breaking change. I don't think that we want to revert to only allowing one child for context providers, as this would impact many use-cases beyond the naming issues we're hitting in Roact Gamepad.

Many libraries, like Roact-Rodux, depend on this functionality and don't have naming issues as they're not higher-order components (HOC).

We have a couple solutions we should consider.

Looser Coupling

In the immediate term, can we stop depending on the exact names of more children? Tightly coupling an integration test to an exact instance tree makes those tests brittle.

Rhodium has XPath queries to help traverse chunks of the tree with more unknown variables. If those queries fall short, we should be able to expand them.

Expose self.key on components

We may be able to expose a component's key. If Roact Gamepad had access to a value like self.key, it could use that key as the key into the provider it creates.

React does not expose this information, but keys in React are not as meaningful as in Roact.

Change fragment key generation

When a component uses fragments, Roact currently uses the given keys as-is. We could instead generate new keys by concatenating the parent key with the key from the fragment container.

For this example, your components might end up with names like LeftFrame-Focusable instead of just Focusable.

If there is other code at Roblox that is tightly coupled to the names of constructed instances, we should make sure we won't break that code.

@ConorGriffin37
Copy link
Member Author

Changing this to use oneChild wouldn't stop people from passing a fragment as that one child and having multiple children but I guess this is a pretty big breaking change to make.

I think perhaps exposing self.key would be the best solution, this might also be useful for debugging purposes. I'm not familiar with the the XPath queries beyond name but fixing it like this would still mean that many objects in the tree would be named Focusable which impacts development in other ways such as making it harder to find an object in the tree with explorer search to inspect it's properties for example.

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

Successfully merging this pull request may close these issues.

None yet

3 participants