Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions types/react/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,29 @@ declare namespace React {
props?: Partial<P> & Attributes,
...children: ReactNode[]): ReactElement<P>;

// Context via RenderProps
interface ProviderProps<T> {
value: T;
children?: ReactNode;
}

interface ConsumerProps<T> {
children: (value: T) => ReactNode;
unstable_observedBits?: number;
}

type Provider<T> = ComponentType<ProviderProps<T>>;
type Consumer<T> = ComponentType<ConsumerProps<T>>;
interface Context<T> {
Provider: Provider<T>;
Consumer: Consumer<T>;
}
function createContext<T>(
defaultValue: T,
calculateChangedBits?: (prev: T, next: T) => number
): Context<T>;
Copy link
Copy Markdown
Member

@Jessidhia Jessidhia Mar 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be possible to call createContext() to create one with defaultValue of undefined.

function createContext<T>(): Context<T|undefined> override? This is one of those that look like the antipattern warned against in the docs, but I think it's correct to use here, similar to a collection constructor.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. I'm confused here. Calling createContext() (i.e., no arguments) would require the defaultValue to be optional. Otherwise you'd need to do an explicit createContext(undefined). Also, since T can be anything, I don't understand T|undefined.

FYI, I address this case of optional default values in my article on the new Context API (using the typings as they stand here, but wrapping them in one additional layer). Also note that, in my opinion (see article), it isn't just about making the default value optional. But again, I used these typings without issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

16.3 released today and the docs seem to indicate @Kovensky is correct here — they use ‘createContext’ without args so it must be a correct usage. I’ll take a look at updating later today.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, rebased this post merge conflict.

Spent some time with this tonight and it looks like the behavior of the final API is that
createContext() gives you a context object where _defaultValue is undefined.

createContext('foo') gives you a context object where _defaultValue is foo

Given that, I'm a bit confused as to the correct typing for createContext() with no params. I tried the overload @Kovensky mentioned but doing so types the resulting RenderProps function args with {} or undefined, which I don't think is correct.

I suspect the overload closest to the API's usage would be

function createContext(): Context<any>

Which completely gives up type safety for RenderProps function but at least allows it to be used. {} or undefined doesn't allow you to still give, say, an anonymous object in the Provider's value field and still be able to retrieve it later in the RenderProps.

If anyone has any opinions on this they'd be welcome.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I'm just saying that we shouldn't want to correct that behavior, because you can always use Consumer not paired with a Provider so the value of T should not change even if you later pass in the value as a prop to Provider.

I.e. if you create the context with the default value, you are always guaranteed to have a defined value wherever you use Consumer (even not as a descendant of Provider), but once you create the context without a default value, you are not guaranteed that defined value unless you wrap the Consumer inside a Provider, which wouldn't be able to be statically analyzed before runtime.

I'm agreeing with you that it's unfortunate that we'll always have to check for undefined, but I think that should be the way the interface is structured due to the lack of compile-time guarantees of the consumer value

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, in your first example you had the comment there /* value here will be defined, even though it's typed as optional */ and that was what's confused me the most. As you have said now, you will have to check for undefined either way. I understand now.

I see a bit of problem arising here. Developers are generally lazy and we might end up with pattern like this.

interface MyContextType { myValue: string }
const Context = createContext<MyContextType>()

<Context.Consumer>{value => { 
  <div>{value!.myValue.toUpperCase()}</div> // <-- notice the "!" mark there
</Context.Consumer>

That might be a good source of runtime errors (and blaming TypeScript/React for it) in case someone really forgets to add the Provider. It's true that if anyone is using ! it's for own risk, but there are people that might just copy&paste and then be surprised.


What about this approach? Would this work the same way? I am not sure if it's a correct syntax, but you get the gist. This would at least allow to override this undefined type in case someone knows what they are doing.

function createContext<T, S = T | undefined>(): Context<S>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm.. it might not be immediately clear to the user why the second generic type param may be used, and they'll have to dig into the type defs to actually find out how they resolve.

With my suggestion (the two overloads), creating context with a default param will assert the value in the consumer to be always be defined:

interface ContextProps {
  foo: string
  bar?: number
}

// note the default values passed in as the first parameter
const Context = createContext<Props>({ foo: 'foo' }) // typed as Props
const OtherContext = createContext({ foo: 'foo' }) // inferred to be type { foo: string }

// even without a Provider it is still typed to be required
const app = (
  <App>
    <Context.Consumer>
      {( value ) => /* value is typed to be Props here, no need to check for undefined */ }
    </Context.Consumer>
    <Context.Consumer>
      {( value ) => /* value is typed to be { foo: string } here, no need to check for undefined */ }
    </Context.Consumer>
  </App>
)

in this case, all you need as a type consumer to override the undefined behavior is to just provide a default value, the type inference will work for you or you can explicitly set the generic type parameter, and as long as the default value is passed in as a parameter, the consumer render props will always be defined

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ferdaber I'm trying to use the TypeScript 2.8 features you mentioned in this definition, but it looks like that's not possible yet?

I've updated the headers to // TypeScript Version: 2.8 but that results in a linting error.

According to this comment, we can only use TypeScript up to 2.7 right now, so I may not be able to use the definition you specify.

#24586 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, that's too bad, we'll keep it in the backburner then

function createContext<T>(): Context<T | undefined>;

function isValidElement<P>(object: {} | null | undefined): object is ReactElement<P>;

const Children: ReactChildren;
Expand Down
3 changes: 3 additions & 0 deletions types/react/test/tsx.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,9 @@ const StatelessComponentWithoutProps: React.SFC = (props) => {
};
<StatelessComponentWithoutProps />;

// React.createContext
const ContextWithRenderProps = React.createContext('defaultValue');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the way too simple test to cover the changes. I would propose following, but I am not entirely sure how to run these tests, so there might some mistake (or several)

interface ContextTest = {
  assert: boolean
}
const ContextWithValue = React.createContext<ContextTest>({ assert: true })
const ContextWithoutValue = React.createContext<ContextTest>()

<ContextWithValue.Context>{value => value.assert}</ContextWithValue.Context>

<ContextWithoutValue.Provider value={{ assert: true }}>
  <ContextWithoutValue.Context>{value => value.assert}</ContextWithoutValue.Context>
</ContextWithoutValue.Provider>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FredyC I'm not sure myself. The test files here don't seem to be assertion-style tests so much as a sandbox to prove that the typings work without syntax errors.

I agree that there could be more code here exercising both context scenarios (with and without defaults), but I don't know that they should be executable as none of the rest of the code in this file seems to be.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well yes, tests code is not executed, because you're testing types, not React's behavior.
But tests shouldn't always be positive, if you think that there might be some usage that is not correct, but could be assumed as correct one.

What you have to do for failing tests is add // $ExpectError line immediately before the test line. More about that in DT readme:
https://github.com/DefinitelyTyped/DefinitelyTyped#lint
And dtslint readme:
https://github.com/Microsoft/dtslint#write-tests

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scally Are you willing to try that? Perhaps it's overkill for such relatively simple thing. It's your call.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FredyC Yes, I'll try it out and see where I get with it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am trying something like this:

const RenderPropsContext = React.createContext('defaultValue');

interface RenderPropsContextTest {
    assert: boolean;
}

const ContextWithDefaultValue = React.createContext<RenderPropsContextTest>({ assert: true });
const ContextWithoutDefaultValue = React.createContext<RenderPropsContextTest>();

const ContextRenderPropsConsumerComponent = (
    <ContextWithDefaultValue.Consumer>
        {value => value.assert}
    </ContextWithDefaultValue.Consumer>
);

const ContextRenderPropsProviderConsumerComponent = (
    <ContextWithoutDefaultValue.Provider value={{ assert: true }}>
        <ContextWithoutDefaultValue.Consumer>
            {value => value.assert}
        </ContextWithoutDefaultValue.Consumer>
    </ContextWithoutDefaultValue.Provider>
); // $ExpectError

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FredyC Are these tests sufficient for what you feel should be achieved here?

Copy link
Copy Markdown
Member

@Jessidhia Jessidhia Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange that the error is in the full statement instead of in the argument to Consumer, but seems to be the case.

An additional non-erroring test where the argument is value => value === undefined ? null : value.assert would also help demonstrate it.

There's one last confirmation I'd like to take; if the Context is created with a default value, will the Consumer receive that value even if it's not put inside its Provider in the tree? 🤔 looks at source

EDIT: The context's context has a copy of the default argument as its value, and it's only overwritten by the reconciler if the Provider is present. It's also restored to its original value once the Provider tree has been processed. So indeed it can only be undefined if you passed something that can be undefined (or nothing) to the createContext call.


// Fragments
<div>
<React.Fragment>
Expand Down