Update constructor docs to include context

[dustinsoftware]

The docs previously did not indicate that it was necessary to call super(props, context) if you are using context within components, which could cause confusion. Opening this PR to start discussion around a change to make this more obvious (I searched and could not find a previous issue)

[reactjs-bot]

Deploy preview ready!

Built with commit 12d8a2d

https://deploy-preview-168--reactjs.netlify.com

[gaearon]

We intentionally don't use context in the API docs anywhere except the Context page because it's still considered experimental and people should minimize its direct usage. Even though we won't remove it we do plan to change it. Most people shouldn't attempt to use it directly.

It would make sense to me to instead add a caveat about this to the Context doc itself, and leave others as they are.

[dustinsoftware]

Ok, I can make an update to that page then :)

…

On Mon, Oct 16, 2017 at 17:04, Dan Abramov ***@***.***> wrote: We intentionally don't use context in the API docs anywhere except the Context page because it's still considered experimental and people should minimize its direct usage. Even though we won't remove it we *do* plan to change it. Most people shouldn't attempt to use it directly. It would make sense to me to instead add a caveat about this to the Context doc itself, and leave others as they are. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#168 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA5hFmWvHHtLwdcdEBlHoMzBKZCF6awZks5ss-72gaJpZM4P7ZX2> .

[dustinsoftware]

I looked, the page on context already has some documentation about the constructor.

Would you be open to adding a warning to the component docs page about avoiding using context directly, which links to the context docs page? It seems like since this page doesn't mention context in the constructor, it would be easy for someone to mistakenly exclude it, which would break using a library like react router, correct?

[gaearon]

I'm open to adding a second "Note" in the constructor doc that mentions context linking to its doc and shows the different signature.

[EthanRutherford]

I would honestly recommend rest/spread-ing the constructor args.

class Foo extends Component {
    constructor(...args) {
        super(...args);
        // ...
    }
    // ...
}

A component author generally shouldn't be fiddling with any of the arguments to the constructor, and writing your constructors this way also future-proofs the component: if React team decides for whatever reason to radically change the constructor arguments, a constructor declared as above will be unaffected by the change. The same argument also applies to migrating a project into react-router, or any other framework which similarly uses context. Existing components should Just Work™️ regardless of the structure of the constructor arguments.

[gaearon]

This makes it sound like you need to use context directly to use React Router which isn't the case. RR ships with components and HOCs that hide the use of context. Can we reword to avoid this impression?

[dustinsoftware]

Sure thing.

[dustinsoftware]

Ok, the wording has been updated

[bvaughn]

I'm a little uneasy about this wording. It's not technically required that you call super with context (or even props for that matter) since React has your back in case you forget. The only time it matters, in practice, is if you call this.context within the constructor function.

I'm wary that this change could influence people to bulk-modify their code unnecessarily in order to change super(props) to super(props, context) and that this could in turn make changes to context slightly less convenient down the road. (Although that kind of change would also be pretty easy to code mod away so maybe I'm just over-thinking it.)

I'm also a bit nervous that this note might confuse people unnecessarily (eg "How do I know if any of the libraries I'm using rely on context?") when in practice, it only matters in a very specific (and probably not super common) situation.

Either way, thanks so much for the proposal and the discussion. I could be wrong - and I'd love for people to speak up if they disagree with this decision - but I think it's best to leave this note out for now.

[dustinsoftware]

Ok, thanks for taking the time to reply. :)

…

On Fri, Nov 17, 2017 at 08:04, Brian Vaughn ***@***.***> wrote: I'm a little uneasy about this wording. It's not technically *required* that you call super with context (or even props for that matter) since React has your back in case you forget <https://github.com/facebook/react/blob/b5e4b45a1036e3587415ae395f9d62a0229ed7e2/packages/react-reconciler/src/ReactFiberClassComponent.js#L446-L449>. The only time it matters, in practice, is if you call this.context *within* the constructor function. I'm wary that this change could influence people to bulk-modify their code unnecessarily in order to change super(props) to super(props, context) and that this could in turn make changes to context slightly less convenient down the road. (Although that kind of change would also be pretty easy to code mod away so maybe I'm just over-thinking it.) I'm also a bit nervous that this note might confuse people unnecessarily (eg "How do I know if any of the libraries I'm using rely on context?") when in practice, it only matters in a very specific (and probably not super common) situation. Either way, thanks so much for the proposal and the discussion. I could be wrong - and I'd love for people to speak up if they disagree with this decision - but I think it's best to leave this note out for now. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#168 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA5hFoZ5EyaVPYkTYs-7EIi-MslQTNXJks5s3a5FgaJpZM4P7ZX2> .