allow returning an array of jsx elements

[donaldpipowitch]

Hi everyone! First time contributor here 👋 At least in terms of the compiler.

It is currently not possible to return an array of JSX Elements inside of a stateless React component. The change can't be applied to the typings directly, because the constructor is affected. See here for an open issue DefinitelyTyped/DefinitelyTyped#20356 and this comment DefinitelyTyped/DefinitelyTyped#19363 (comment).

I basically want to be able to change (props: P & { children?: ReactNode }, context?: any): ReactElement<any> | null; of the stateless component to
(props: P & { children?: ReactNode }, context?: any): ReactElement<any>[] | ReactElement<any>| null;.

I looked for older similar issues and found this one: #14152. (Surprise. This is actually a fix for an older issue I reported, because I couldn't return null. 😄 #11566) (Note that it's hard to initially find the correct code via GitHubs search, because the checker.ts is too big. It looks like GitHub doesn't allow searching in big files. I usually start by finding and reading the code on GitHub, before I clone the repo to get some initial impression if it is fixable for me.)

So here I am. This is an ongoing PR. If you have any feedback, I'd be happy. Is this even the right place to make the change?

I'll start looking into tests as the next step.

[donaldpipowitch]

So if I see correctly I should now create the following files:

• tests/baselines/reference/tsxSfcReturnArray.js
• tests/baselines/reference/tsxSfcReturnArray.symbols
• tests/baselines/reference/tsxSfcReturnArray.types
• tests/cases/conformance/jsx/tsxSfcReturnArray.tsx

Are the .symbols and .types files written by hand or generated?

[gcnew]

You should write only the .ts / .tsx file. The others are derivates generated by the testing logic. Just add your tests where appropriate under tests/cases and run the tests through jake/grunt. A baseline will be created, which upon verification you should accept.

PS: If you haven't already, you can read on the needed commands in the Contributing How-To.

[msftclas]

All CLA requirements met.

[donaldpipowitch]

Thank you @gcnew. $ jake runtests tests=tsxSfcReturnArray seemed to run fine 👍

I also linked my changes (also applied to the lib/** files) and to a local project with (props: P & { children?: ReactNode }, context?: any): ReactElement<any>[] | ReactElement<any>| null; for stateless components and it seemed to build fine 👍

[Jessidhia]

This would warn at runtime due to missing key on the elements, but does it matter? :)

[donaldpipowitch]

You're right, this would warn at runtime. I guess it is fine for the test here as it only checks compiling...?

[donaldpipowitch]

Hi @Kovensky, thank you for the review. Anything I can do to get this merged? :) Thanks!

[RyanCavanaugh]

Can you clarify a bit on why it's not sufficient to update the react.d.ts file?

[donaldpipowitch]

I think it is a similar problem to this one: #14152
The return type for the JSX constructor is different and (AFAIK) this needs to be fixed in TS itself or you'd get an error like JSX element type 'ReactElement | ReactElement[] | null' is not a constructor function for JSX elements..

[mhegazy]

@RyanCavanaugh and @weswigham what is the final call on this change?

[weswigham]

IMO, we could do more here since, for example, all elements returned in the array must have the key field definitely assigned (meaning the array element type would more correctly be JSX.Element & { key: string } to force the key field to be set, this could be in the react .d.ts). This is also only react 16+, so adding this will reduce safety for users on older react versions (unlike the addition of fragments, which will error if the type is not present). I also think this could oddly could change our behavior in the edge case where someone has a react component that inherits from array (which I believe was perfectly valid pre-react 16). We really should be looking at a way to pull this type from the react .d.ts we load. So while this works as is (though I'd merge with master and double check that an array of generic elements is returned/inferred correctly), I'm not really happy with the current state of affairs here (nor with how #14152 was implemented in the end, actually, since it effectively hardcoded part of the react .d.ts and assumptions thereof into our checker without expecting anything new in the JSX namespace). We should probably be looking up the SFC type by checking for call signatures of the first parameter type of all signatures of the factory function and unioning those signatures' returns all together, but that's a general change to how we should handle jsx stuff.

TL:DR: This looks good (pending update with master and ideally a test with generic components), but I'm not happy about it, because it has the possibility of quietly reducing safety for people on older react versions (and strongly couples us to react 16+ in general). We should fix that.

[weswigham]

Also note, you can render strings directly now, too:

render() {
  return 'Look ma, no spans!';
}

This should be able to support that. (Also numbers, booleans, and portals)

[donaldpipowitch]

I agree, I'd prefer to not include so much React-specific and React-version-specific information directly in the compiler as well. It'd be great if this PR could just have been a PR to DefinitelyTyped instead. For now I'll try to

• update with master
• add test with generic component (Do you have an example for this? Would be great.)

I'd keep all other points open for future PRs. If someone helps me finding the correct strategy I could try to also remove the React-specific parts in the compiler and put them to a different place...?

[donaldpipowitch]

Maybe TypeScript can support a new sort of primitive/plugin which enables extracting the React-JSX-logic into a standalone package. E.g. the current TypeScript Language Service Plugins don't allow adding new custom syntax to TypeScript, but maybe we can have "TypeScript Language Syntax Plugins" or "TypeScript Language Macros" or whatever in the future where this would be possible...?

[weswigham]

Hmmmm... So we talked about this a bit today in person, and we'd like to take this opportunity to remove this coupling with react, so nobody needs to go in and update us when react changes (and to avoid breaking older react versions). The correct type to check here should be the return type of each of the signatures if the first argument type of the factory function, for all factory function signatures whose first argument type has call or construct signatures.

[weswigham]

*the return type for the call signatures, the return type of the render method of the return type of construct signatures.

[donaldpipowitch]

Could you point me to the places which would be needed to change? And maybe some pseudo code for an overview which needs to be done? That would be really nice as this was my first PR to TypeScript ever and I'm not very familiar with the code base. Then I'd try a new PR.

So what we'll do is basically is extracting the return types so they can be configured externally, while the while JSX parsing logic stays in TypeScript, right?

[danielkcz]

I have recently run into this issue when I tried returning a string from SFC (stateless functional component). There is a PR pending in DefinitelyTyped repo, but it's failing there and apparently, it's nothing we can do about it.

I can just add my two cents that extracting React specific logic out of the Typescript seems reasonable way, but I have no clue how that could be approached. Typescript does not have any sort of plugins or extensions, or does it?

[weswigham]

@donaldpipowitch sorry this has taken a bit for me to writeup, but it does make the change a fair bit more involved, compared to what's here 🙁

First, inside checkJsxOpeningLikeElementOrOpeningFragment we have some logic for looking up the jsx factory-namespace symbol and marking it as referenced. First, you'd need to extract that symbol-finding code into a new function so you can reuse it elsewhere. Next, you'll need to create another function that, given a location, looks up the factory function symbol. This involves using the above function, and, if _jsxFactoryEntity is more than a single identifier (this is initialized by getJsxNamespace), traversing the symbol's type to get the factory function type corresponding to that entity name (this would use getTypeOfSymbol and getPropertyOfType). Then, you'd make another function which, given a location, uses the prior function to calculate the stateless function type of that factory function. To do so, you would get the call signatures of its' type (using getSignaturesOfType with its type and SignatureKind.Call), then map those signatures to the type of their first parameter (using getTypeAtPosition on the signature), then filter that list down to those types which are function types (using isFunctionType), then, finally, you get the call signatures of all those types (same as before), and map all those call signatures into their return types (and then instantiate these types with their constraints using instnatiateType with a custom mapper that maps t => t.flags & TypeFlags.TypeParameter ? getBaseConstraintOfType(t) || emptyObjectType : t until we manage to enable inference during this process, likely by moving some work into getJsxElementInstanceType or moving inference out of that function), then call getUnionType on that list of types - this is the jsx stateless element type. Now everywhere we call getJsxGlobalStatelessElementType, call your new function instead, supplying an appropriate location (usually the jsx tag we're looking at).

Ideally we'd then do a similar thing for ElementType, except we'd check for constructor types instead of call types, and instead of looking at the return type directly, looking at the return type of the render method of the return type.

There's a long trail of ways to improve (and associated issues), so I think I'll refrain from suggesting anything more here. 🐱

[donaldpipowitch]

Really thank you for this summary. I'll see when I can find time to fiddle around with this. Should this PR be closed for now or merged as it is? Just so everybody knows where we currently are.

[weswigham]

@donaldpipowitch I've opened an issue to track the problem - you can close this PR. If you aren't able to get to this in the next week or so, we'll probably take it on internally, since we do really want to see this fixed by 2.8, now that we know about the problem.

[donaldpipowitch]

@weswigham Ah, I see. I don't think I can fix this within a week. I have a couple of private events and less spare time currently. Feel free to start with it now.