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

[clehene]

Note that I added a PR to update pitfalls.ms mobxjs/mobx#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.

mobx-react/src/observer.js

Line 24 in 12cce61

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

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

[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]

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.