Skip to content

feat(devtools): Allow editing of context value in Consumer #18257

New issue

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

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

Already on GitHub? # to your account

Closed

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Mar 9, 2020

Review on per-commit basis advised

Summary

Allow editing of the context value from a consumer if the value comes from a Provider component.

Test Plan

Verified locally by using react-devtools-shell:

master:
devools-context-consumer-master

pr:
devools-context-consumer-pr

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 9, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1e301d0:

Sandbox Source
frosty-surf-b44j9 Configuration

@sizebot
Copy link

sizebot commented Mar 9, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 1e301d0

@sizebot
Copy link

sizebot commented Mar 9, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 1e301d0

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

<Consumer> is effectively a mostly legacy API now that we have useContext and class contextType. If we're going to implement this feature, I think it only makes sense to implement there.

I don't think letting you edit from Consumer is the right UI regardless. I think maybe it should offer a "link" to jump to the appropriate Provider. So that you can see which one manages it and that it's higher up the tree.

Then in the Provider maybe it does make sense to edit it. Although at that point you might as well just edit the appropriate state? So perhaps it's really the "jump to provider" feature that would be most useful.

@eps1lon
Copy link
Collaborator Author

eps1lon commented Apr 1, 2020

I think maybe it should offer a "link" to jump to the appropriate Provider.

Yep, I was thinking this as well. Happy to abandon this one in favor of a "jump to provider" feature combined with #18255

Although at that point you might as well just edit the appropriate state?

You mean like

function App() {
  const [state] = React.useState({ color: 2 }); // <--- edit this hook instead?
  return <Context.Provider value={value} />
}

It's not always obvious where the value of a provider comes from. Could be a memo or useState or useReducer hook. I think editing the value directly is more straight forward.

@eps1lon
Copy link
Collaborator Author

eps1lon commented Apr 1, 2020

But I do feel like that being able to edit context in the context of the component is still important (useContext) here. Otherwise it's not easy to jump between the component props and the context for that particular component.

@stale
Copy link

stale bot commented Jul 3, 2020

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jul 3, 2020
@eps1lon
Copy link
Collaborator Author

eps1lon commented Jul 3, 2020

bump

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jul 3, 2020
@stale
Copy link

stale bot commented Oct 4, 2020

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Oct 4, 2020
@eps1lon
Copy link
Collaborator Author

eps1lon commented Oct 4, 2020

Abandoning. Context.Consumer isn't that important. useContext would be more helpful but is more complicate last I looked.

@eps1lon eps1lon closed this Oct 4, 2020
@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Oct 4, 2020
@eps1lon eps1lon deleted the fix/devtools/context-editable-consumer branch October 4, 2020 07:44
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants