search

mobxjs / mobx-react

React bindings for MobX
https://mobx.js.org/react-integration.html
MIT License
4.85k stars 349 forks source link

decorator @inject not works with typescript and 'reflect-metadata' previously: #756 #857

Closed binaryangel-noa closed 4 years ago

binaryangel-noa commented 4 years ago

"decorator @inject not works with typescript and 'reflect-metadata' #756" has been closed. My team now started migrating our v5 application to v6 and we have the same error. The Documentation still has this way of using using decorators as a valid way and the migration documentation states that its possible to gradually migrate to v6 of mobx react. Thats also the strategy we follow with react right now (with classes to functions).

Whats the way for migrating from the old pattern without having to rewrite every class component ?

mweststrate commented 4 years ago

export const X = observer(class X) rather than @observer class X

On Wed, Apr 29, 2020 at 3:58 PM binaryangel-noa notifications@github.com wrote:

"decorator @Inject https://github.com/Inject not works with typescript and 'reflect-metadata' #756 https://github.com/mobxjs/mobx-react/issues/756" has been closed. My team now started migrating our v5 application to v6 and we have the same error. The Documentation still has this way of using using decorators as a valid way and the migration documentation states that its possible to gradually migrate to v6 of mobx react. Thats also the strategy we follow with react right now (with classes to functions).

Whats the way for migrating from the old pattern without having to rewrite every class component ?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mobxjs/mobx-react/issues/857, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAN4NBALFTCWWWBFNURSUK3RPA6DBANCNFSM4MTZOCEQ .

danielkcz commented 4 years ago

Have you read through https://mobx-react.js.org/recipes-migration? Any help is welcome in improving it if something is unclear there. Rather make a PR than explain what's wrong there, please.

binaryangel-noa commented 4 years ago

Hi, yes i have, https://github.com/mobxjs/mobx-react still uses @inject approach in its documentation which let me suspect its a bug or forgotten feature. If it was intentional to remove that option then the the migration documentation is missing to mention that @inject as decorator does not work anymore and as a result components using that need to be rewritten to: class Example { } export default inject("storename")(Example)

Also "First and foremost: if you are happy with the current Provider/inject pattern, there is nothing you need to change in your application. This guide is intended as the path to the future of React and tools that might provide you with more comfort." does not hold true in that case.

What do you mean by PR ? I will try to provide those information's there.

danielkcz commented 4 years ago

The @inject does work, just not in your specific case with reflect-metadata. We can't cover all edge cases in docs, it's already long as it is :)

The inject/Provider got reworked in V6 to utilize React.Context. It's totally possible something got lost on the way to support reflect-metadata.

If you don't want to migrate everything for now then I would recommend you downgrade back to V5 and use mobx-react-lite alongside for components refactored to functional ones. When you are done with the migration, it will be as simple as replacing one string for another.

What do you mean by PR ? I will try to provide those information's there.

For improving docs. It's always helpful for others to document hurdles you had to overcome on the migration path.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.