search

zooniverse / Panoptes-Front-End

Front end for zooniverse/Panoptes
https://www.zooniverse.org
Apache License 2.0
65 stars 76 forks source link

Remove `context` api from PFE. #4281

Closed simoneduca closed 6 years ago

simoneduca commented 6 years ago

Expected behavior

Use props.store to access redux store, instead of context.store. The reason being that context is explicitly not recommended by React docs.

Here's a list of all components which currently use connect(), and would need to be updated to receive the store as a prop.

simoneduca commented 6 years ago

I'll test this out on one component to see how much effort is required and then decide if it's worth it.

eatyourgreens commented 6 years ago

Tutorials are probably the most complicated example, because they use static methods. https://github.com/zooniverse/Panoptes-Front-End/blob/f632f0800122c8d7e606f6e2c9fd82b59ab4740a/app/classifier/index.cjsx#L66

Also, looking through that file, I see that I've missed adding the store argument in ComponentWillReceiveProps.

eatyourgreens commented 6 years ago

Worth noting that the react-redux docs recommend against using props except as a last resort.

If you really need to, you can manually pass store as a prop to every connect()ed component, but we only recommend to do this for stubbing store in unit tests, or in non-fully-React codebases. Normally, you should just use <Provider>. https://github.com/reactjs/react-redux/blob/master/docs/api.md#provider-store

simoneduca commented 6 years ago

PFE seems to fit the last resort scenario, no? Because the error message returned when store is undefined says: Uncaught Error: Could not find "store" in either the context or props of "Connect(TaskTranslations)". Either wrap the root component in a <Provider>, or explicitly pass "store" as a prop to "Connect(TaskTranslations)". It does not say use context manually/explicitly.

eatyourgreens commented 6 years ago

Master, at the moment, explicitly passes the store as a prop but wrapping modal content in<Provider store={this.context.store} > would also work. See the advice about dealing with context errors towards the bottom of the troubleshooting page: https://github.com/reactjs/react-redux/blob/master/docs/troubleshooting.md

In fact #4280 works because PassContext is being used as a Provider in that PR.

srallen commented 6 years ago

Closing since the context API is now properly finalized, documented, and supported in React 16. Specific refactoring for the newer API should have separate issues opened for it if it needs to be done.