Lookup return type of factory function for JSX expression return types #29818
Conversation
src/compiler/checker.ts
Outdated
| links.jsxFactoryCall.end = node.end; | ||
| links.jsxFactoryCall.parent = node.parent; | ||
| } | ||
| const result = getReturnTypeOfSignature(getResolvedSignature(links.jsxFactoryCall)); |
There was a problem hiding this comment.
Could this potentially let us get rid of JSX.Element (or, heck, the entire JSX namespace)? 🤔
If we can do that and redefine createElement / React.ReactElement we might be able to get strong type checking for element children, and more 🎉
sounds like an excellent opportunity for a breaking change, almost all of the overloads in createElement were obsoleted by conditional types too and only can't be fixed because it'd be breaking
There was a problem hiding this comment.
That would be what one would hope for, yeah. Technically we don't want to be rid of it quite yet because there's no way to define the factory function that correctly resolves overloads on components (or generics), but by and large if we had those this wouldn't need the namespace at all.
|
So would this essentially make the following equivalent as far as type compatibility goes? // @jsx factoryFunction
const foo = <MyComponent someProps={myProps} ref={myRef}>{myChildren}</ MyComponent>
const foo2 = factoryFunction(MyComponent, { someProps, ref }, myChildren) |
|
@typescript-bot run dt |
|
Heya @weswigham, I've started to run the Definitely Typed test suite on this PR at f74105a. You can monitor the build here. It should now contribute to this PR's status checks. |
weswigham
force-pushed
the
jsx-element-realtypes
branch
from
60feb37
to
cec5c8d
Compare
|
I think this change is actually pretty good - @typescript-bot run dt again - I didn't see any I would love for some more folks to run this PR on their own jsx codebases and report the perf impact if possible - running |
|
Heya @weswigham, I've started to run the Definitely Typed test suite on this PR at cec5c8d. You can monitor the build here. It should now contribute to this PR's status checks. |
|
@weswigham I can volunteer if you can guide how to get this specific version of TS. |
|
@typescript-bot pack this |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at cec5c8d. You can monitor the build here. It should now contribute to this PR's status checks. |
|
@ferdaber |
|
Any specific workflows you'd like me to test? |
|
Just checking your codebase - running with the old version of TS with |
|
@weswigham we have ~400 *.tsx files. |
beforeafterSo we have 6 new errors, which are either tsc regressions or errors on our part we would have to fix. Other than that the check time seems to be faster by a bit. |
|
Are the new errors unique to this branch, or do they appear on |
|
tried Same 6 errors as with this insiders build. 4 of them look related to https://github.com/eversport/intl-codegen and https://github.com/eversport/pyro-form, two of our own libraries we also open-sourced, so we would have to look into those errors. But they are JSX related. The 2 remaining errors look like |
|
Just for reference, but since its the same error as in |
|
Hmmm, I'd greatly appreciate if you could look into if those new errors make sense and provide a repro (and open an issue) if they don't. But yeah, not related to this PR if they happen on @Jessidhia what changes were you thinking of applying to the react declarations? |
|
I think this will be an amazing addition to TS, and will provide the ability for new ways to use TSX! What is left to get this finalized? |
More testing so we have more confidence, thanks. 🚂 We won't ship this in 3.4 (we already cut the RC), but that means this is one of the first things up for inclusion in 3.5 in two weeks. If we can get sufficient attestations that this doesn't impact large react projects poorly by then, that'd be great. We're especially interested in super-large projects that already take close to or over a minute to build - those are the ones we're especially worried about. I'll have @typescript-bot pack this so we can get a new package drop that's up-to-date with current |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at 5b9817b. You can monitor the build here. It should now contribute to this PR's status checks. |
|
|
weswigham
changed the title
|
@typescript-bot test this & @typescript-bot perf test this (though none of our perf tests contain JSX so i doubt they'll show anything useful) |
|
Heya @weswigham, I've started to run the extended test suite on this PR at ebc8fce. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at ebc8fce. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - master..29818
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Yeah, there was an issue in |
|
I was able to build and run the branch locally. I got very similar results to #29818 (comment) (I need to increase memory to run it successfully, I get many errors, and it took longer). With this branch: With TypeScript 3.5.3: |
weswigham
added
the
Experiment
|
I am not sure what the status of this is, but since it's required to fix these issues... DefinitelyTyped/DefinitelyTyped#33908 ...I'm wondering if we can get a general update? |
|
I've been working on a virtual-dom library that uses Generator Functions to represent Algebraic Effects for asynchronously rendering components. I'd find it very helpful to be able to use the JSX factory return type to properly type values and not require all of my generics to be filled in with I'd like to help see this across the finish line if there's anything helpful I could still do at this point. |
|
Well, so this works, but it has 2 problems:
let a = <div></div>;
a = <img />;ends up being an error because rather than both being
|
|
Hey @weswigham, thanks for the reply. Excuse any of my ignorance, but would it be possible for a solution to 1 be to place this feature behind a new JSX-related compiler flag? Maybe something like As for 2, I'll have to get myself more acquainted with how this functions right now before. Have there been any prior discussions on what optimizations remain to be made? |
|
This performance (1kLoC/s) sounds like fair price for having JSX properly typed. In my personal project I have to use custom AFAI understand, performance is affected a lot by number of overloads, and it can get way better if library users choose a subset of those, because barely any real-life projects require all the tags from If the performance problem is still an issue after that, it's always possible to resort to separate compilation (restrict dependencies to be a tree, build js/d.ts pair from each ts file with a set d.ts for its imports), even though I haven't seen this approach used anywhere. I can try it on a large project at job, if you still need more input on its performance, but I suspect it won't be able to fit in memory. |
|
Wouldn't it be possible to release this feature as optional in an upcoming version? As @TylorS says, the compiler flag sounds like a good solution. Every time I work with TypeScript and any Virtual DOM library, I find myself needing this feature. And every time it's terribly frustrating to know it's already implemented and I just can't use it. Thank you for your work on this feature, but please consider merging it again. JSX is an awesome concept, but without type-checking it's as unfriendly as pure JavaScript without TS. EDIT: I think many of the performance issues might be mitigated by skillfully typing EDIT 2: It should be noted that needing typed JSX in React should be a relative edge case. The philosophy of React is that parent nodes shouldn't do any magic with their children, the children should always decide for themselves. That, however, doen't mean there aren't valid uses for typed JSX in React. And, most importantly, in other non-React libraries that use JSX, there isn't this kind of precedent, and limiting the type of your child nodes is a totally reasonable thing to do. |
|
I would like to test this feature as well. What I already figured out (is this the right approach?): , which uses Is there also a React types side of things, so we can play around with custom I think, if there would be a more detailed manual, a starter or even an experimental TS flag, more people would be able to test and contribute benchmark results. |
|
Agreed, it would be great to have this behind an experimental flag. I'm still using Flow, but I'd love to take the opportunity to switch to TypeScript -- this is pretty much the only thing blocking me, since I'm extensively using jsx with custom renderers, using https://github.com/krakenjs/jsx-pragmatic |
|
Sorry for the noise, but just adding that this is a really useful feature that I'd really like to see merged. In the meantime I'll be using this branch and see how far I can go. I really like @TylorS's idea of putting it under a seperate compiler option. Would this be feasible? |
|
A friendly bump. This is a quality of life feature, very much anticipated. Since there's a working PR for it already, why holding it back? Hiding it behind a flag sounds like a good idea. |
|
@weswigham you said in #29818 (comment):
Is there a tracking issue or something of the sort for that work? I'm trying to get a sense for what would be involved in getting this over the finish line. FWIW, the use-case I have for this is to strictly-type XML in accordance with a specific schema. |
|
I'd like to make another bump here on this feature. In 2022, this is the #1 feature I personally feel is still missing from TypeScript. I've been using it for 6+ years and it's gotten soooo much better but I keep coming back to this thread and hoping to have seen movement. Primarily I've been wanting to be able to keep track of the resources that a Component needs to be able to run, as in performing dependency injection. I'd hope to be able to aggregate all the requirements "up" to the entry point of my applications and provide all the necessary resources there based on the environment (e.g. node vs browser) to write a component that runs easily in each all while not being tied to any particular view/templating library. I've circumnavigated it in multiple ways over time, like @polkovnikov-ph mentioned with writing hyperscript manually which is often tedious, and to avoid templates requiring dependencies altogether and lifting requirements "up" in various other ways which often leads to more boilerplate to nest or compose components, but I always come back to thinking it'd just be great to have JSX capable of supporting generics itself and utilize a diff like React does to mount/unmount components as needed. Is there any hope of seeing this feature make its way into TypeScript or is the performance overhead more likely to be too much to overcome? |
Three years after, I still don't understand this consequent. Making it work (under a flag) doesn't necessarily require good performance. Whether it could be improved at some point in future or not doesn't really matter, because some of us do need typed JSX anyway. Maybe the actual issue is it's unclear how exactly that flag should be implemented, so that there is no need to provide separate typings for libraries for different JSX typechecker behaviors? |
|
This experiment is pretty old, so I'm going to close it to reduce the number of open PRs. |
Fixes #21699
Fixes #14729 mostly
This is a small change, but with potentially high performance implications.
Done-ish:
Maybe worth doing?:
createElementconstructor (complete with overloads).