Uh oh!
There was an error while loading. Please reload this page.
Auto-include types for the jsx import source in the new jsx transforms - #41330
Conversation
Wesley Wigham (weswigham)
commented
Oct 29, 2020
TypeScript Bot (@typescript-bot) pack this |
Heya Wesley Wigham (@weswigham), I've started to run the tarball bundle task on this PR at 6714998. You can monitor the build here. |
Heya Wesley Wigham (@weswigham), I've started to run the extended test suite on this PR at 6714998. You can monitor the build here. |
Heya Wesley Wigham (@weswigham), I've started to run the parallelized Definitely Typed test suite on this PR at 6714998. You can monitor the build here. |
Heya Wesley Wigham (@weswigham), I've started to run the perf test suite on this PR at 6714998. You can monitor the build here. Update: The results are in! |
Heya Wesley Wigham (@weswigham), I've started to run the parallelized community code test suite on this PR at 6714998. You can monitor the build here. |
Hey Wesley Wigham (@weswigham), I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build. |
TypeScript Bot (typescript-bot)
commented
Oct 29, 2020
Wesley Wigham (@weswigham) Here they are:Comparison Report - master..41330
System
Hosts
Scenarios
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
commented
Oct 29, 2020
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
Stefan Baumgartner (ddprrt)
commented
Oct 29, 2020
Oh cool, thank you! Is this something we should carry over to Preact? (I assume it's similar to tests/cases/compiler/jsxNamespaceImplicitImportJSXNamespace.tsx) -- I can work on a PR and maybe Marvin Hagemeister (@marvinhagemeister) is willing to guide me 😄 |
Yeah, probably! For the implicitly added imports, the full factory is |
Nathan Shively-Sanders (sandersn)
left a comment
There was a problem hiding this comment.
I looked at the checker changes and they seem reasonable. I don't know enough about the program.ts change, but the comment's explanation makes sense to me.
Orta Therox (orta)
left a comment
There was a problem hiding this comment.
Mostly looked over the test cases, and this works like I'd expect 👍🏻
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…mental test for changing jsxImportSource
Wesley Wigham (weswigham)
commented
Oct 30, 2020
Sheetal Nandi (@sheetalkamat) I've added the affixes to the compiler options to make them affect incremental emit, and added an incremental emit test. |
| if (jsxImport) { | ||
| // synthesize `import "base/jsx-runtime"` declaration | ||
| imports ||= []; | ||
| imports.push(createSyntheticImport(jsxImport, file)); |
Uh oh!
There was an error while loading. Please reload this page.
| { path: `${project}/index.tsx`, content: `export const App = () => <div propA={true}></div>;` }, | ||
| { path: configFile.path, content: JSON.stringify({ compilerOptions: jsxImportSourceOptions }) } | ||
| ], | ||
| modifyFs: host => host.writeFile(configFile.path, JSON.stringify({ compilerOptions: { ...jsxImportSourceOptions, jsxImportSource: "preact" } })) |
There was a problem hiding this comment.
Can you add another case hen preact*.d.ts file is missing and added.
And another when it is present and as incremental change goes missing.
Co-authored-by: Sheetal Nandi <shkamat@microsoft.com>
btw I'm assuming I should wait on this for 4.1 RC - is that reasonable? |
Wesley Wigham (weswigham)
commented
Oct 30, 2020
Yeah, I'd rather this get into the RC than go out in 4.1 post-RC, since it affects the usability of the |
Sheetal Nandi (sheetalkamat)
commented
Oct 30, 2020
Wesley Wigham (@weswigham) the change now looks good and adding tests for #41330 (comment) should make it ready to merge |
If we can get the tests in and passing as above, I'll merge this in and kick off the appropriate builds. |
Sheetal Nandi (sheetalkamat)
left a comment
There was a problem hiding this comment.
discussed offline with Wesley Wigham (@weswigham) that there is existing bug with incremental build with missing react files being available later which he will work on later and is not related to this change.
Wesley Wigham (weswigham)
commented
Oct 30, 2020
(specifically there's an issue with synthetic |
So what's the idea? Cherry-pick that to 4.1 but leave it out of 4.1 RC? |
Wesley Wigham (weswigham)
commented
Oct 31, 2020
Well, I got a fix for that underlying issue together pretty quickly, since the issue was actually pretty trivial - our public API sanitity checks forbid answering questions on synthetic declarations like those we'd made - #41346 is out. Up to you. If you can get a review from people you're happy with in a timeframe you're OK with, it can make it into the RC, otherwise saving the fix for post-RC cherry-pick is fine. |
Mateusz Burzyński (Andarist)
commented
Nov 9, 2020
Wesley Wigham (@weswigham) is there a way to make this work: import{createElement}from'react'import{EmotionJSX}from'./jsx-namespace'exportconstjsx: typeofcreateElementexportnamespacejsx{exportimportJSX=EmotionJSX}It complains about "Cannot redeclare block-scoped variable 'jsx'.ts(2451)" even though this works so it's clearly that I've tried to make it work for several hours already and can't do it so far. |
Wesley Wigham (weswigham)
commented
Nov 9, 2020
It's a limitation of our alias-following logic in the binder, if I'm reading it right. If the importtype{EmotionJSX}from'./jsx-namespace'would cause it to work, but I don't know if we adjusted the alias following logic in the binder to allow that to work when we added |
Mateusz Burzyński (Andarist)
commented
Nov 10, 2020
Wesley Wigham (@weswigham) thanks for the tip! I'm afraid it doesn't work though. It also gives me one additional error and that is:
If I find the time I will create a repro case for this and report the appropriate issue. In the meantime I've filled a followup PR to this one: #41476 , please take a look at it when you have time, would be great to fix the issue mentioned there before the final release. |
Fixes#41118
Fixes#40502
A note on #41118 for Stefan Baumgartner (@ddprrt) -
preact's source, in that case, is currently (re)exporting aJSXnamespace under each factory function (multiple factory functions are used within a compilation). That's not quite what I'm looking for, for local JSX namespaces on implicit jsx runtime imports with this PR - a single top-levelJSXexport, alongside all those factory functions, is instead what I want to find. This should simplify the file a bit, as a singleexport {JSXInternal as JSX} from "../../"should suffice.