"Please apply 'observer' before applying 'inject'" should be an error not a warning

[clehene]

Note that I added a PR to update pitfalls.ms https://github.com/mobxjs/mobx/pull/2226 However, I think this is not visible enough. This should throw an exception, unless there's a legitimate use-case to not do it. I spent several hours debugging babel to understand why react doens't re-render on observable changes.

You are trying to use 'observer' on a component that already has 'inject'. Please apply 'observer' before applying 'inject'

[danielkcz]

You are right an exception would be probably more visible, not sure what was the reasoning for the warning. Let's discuss it.

https://github.com/mobxjs/mobx-react/blob/12cce61d3dbaa0ee28d42edcdef43f727d20575d/src/observer.js#L24

I suppose we could show a component name and the isMobxInjector could be carrying the component name as well instead of just a boolean flag.

cc @mweststrate @xaviergonz @vkrol

[danielkcz]

@clehene Just in case you have missed it, but we are slowly moving away from inject pattern. It won't be removed anytime soon, but if you are writing some new-ish code, consider migrating away from it.

https://mobx-react.js.org/recipes-migration

[clehene]

@clehene Just in case you have missed it, but we are slowly moving away from inject pattern. It won't be removed anytime soon, but if you are writing some new-ish code, consider migrating away from it.

https://mobx-react.js.org/recipes-migration

I didn't know. Thank you for the tip. That's a good read. One issue we've faced with FCs was that we started getting warnings about nested hooks (react-hooks-nesting) when using them inside wrapped components.

inject('store')(observer(..
  useEffect(...)
...
));

[danielkcz]

Is that some ESLint rule? I am not familiar with it. Please elaborate in a separate issue, ideally with reproduction and let's keep this one focused on OP.

[danielkcz]

@clehene Given that you seem to be the only one bothered by this problem, let's leave it like this unless you are willing to work on the PR. It's probably not worth the effort due to obsolete pattern.

[clehene]

@FredyC hey - is a throw instead of console.warn ok? I think it affects more than myself once :) (we're also moving away towards contexts). If that's the only change, I'll make a quick PR

[danielkcz]

Yea, that should be enough, along with tweaking tests I would expect.

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs or questions.