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

fixes HOC displayName for observer and inject #472

Merged
merged 1 commit into from Mar 10, 2019

Conversation

SimeonC
Copy link
Contributor

@SimeonC SimeonC commented May 7, 2018

Uses the convention for HOC display names as outlined in the React documentation.
https://reactjs.org/docs/higher-order-components.html#convention-wrap-the-display-name-for-easy-debugging

Closes #466 <- See for more details.

Uses the convention for HOC display names as outlined in the React documentation.

Closes mobxjs#466
@mweststrate
Copy link
Member

I am not sure about wrapping observer() around non-function components; as for normal components it rather acts as mixing then as HoC. (And I think it will be make the entire component tree probably quite unnecessary very verbose?)

The inject names are definitely better this way.

I marked it as breaking change, as I think it could impact test suites relying on snapshots quite a bit. Not really sure whether that counts as 'breaking' though as there should be no runtime impact. But probably this is best shipped as separate minor at least

@SimeonC
Copy link
Contributor Author

SimeonC commented Jun 5, 2018

Yea, it probably is a breaking change for that reason.
My thinking adding the HOC convention to observer was that it would be useful to see which components would be reacting to mobx changes which currently isn't very visible. Didn't think about whether it would be too verbose with it in but it has caught me out every now and then - it also would help to highlight the difference between inject('store') and inject(() => {}) which only one of these applies observer and the other doesn't.

@mweststrate mweststrate changed the base branch from master to v6 February 19, 2019 15:27
@mweststrate mweststrate merged commit 998b2e6 into mobxjs:v6 Mar 10, 2019
@mweststrate
Copy link
Member

Merged into the v6 branch

@github-actions github-actions bot mentioned this pull request Oct 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants