Fix/jsx global preferred over config implicit #41476

Andarist · 2020-11-10T14:44:24Z

Please forgive me for removing a template but at this stage, this is a WIP PR and the template was just off for this right now.

This is also not fixing any reported issue (to the best of my knowledge) because I didn't want to raise one before investigating this further. I've spent quite a bit of time yesterday investigating this and I've prepared more than for a regular issue so the PR format IMHO suits this "report"/fix in progress.

This is a followup to #41330 . When trying to implement this JSX namespace for Emotion I've stumbled upon some problems - didn't know if I'm just doing it wrong or if the logic in TS is broken I've dived into preparing a small repro case and debugging this.

The repro case can be found on the commit here. When executing tsc over a simple file of:

export default <div css="fas" />;

with a simple config of:

{
    "compilerOptions": {
        "strict": true,
        "jsx": "react-jsx",
        "jsxImportSource": "@emotion/react",
    }
}

we get such an error:

src/index.tsx:2:21 - error TS2322: Type '{ css: string; }' is not assignable to type 'DetailedHTMLProps<HTMLAttributes<HTMLDivElement>, HTMLDivElement>'.
  Property 'css' does not exist on type 'DetailedHTMLProps<HTMLAttributes<HTMLDivElement>, HTMLDivElement>'.

2 export default <div css="fas" />;
                      ~~~

This is because the correct JSX namespace has failed to be found.

So after a debugging session, I've discovered that the search process for that namespace is broken because the global React namespace is preferred over the one configured in the config:
https://github.com/microsoft/TypeScript/blob/d72829757de7d496908e8ba52088e11f7c8613d4/src/compiler/checker.ts#L25186-L25190
In here we can notice that getJsxNamespaceContainerForImplicitImport is called last, after resolveName that looks for whatever getJsxNamespace returns and it returns "React" by default:
https://github.com/microsoft/TypeScript/blob/d72829757de7d496908e8ba52088e11f7c8613d4/src/compiler/checker.ts#L1026-L1042
So this has a chance to resolve with React's namespace before we even attempt to look at the "jsx namespace container for implicit return".

IMHO this priority order should not work like this and the one configured using tsconfig.json should be preferred.

I've prepared a simple fix for this which proves to be fixing the issue when I apply the patch on the previously mentioned repro case:
https://github.com/Andarist/emotion-11-automatic-runtime-ts-test/tree/801a5941ba685459ed0809756d2ad665ef907e29

However - I've failed to write a failing test in the repo here. For some reason, this behaves in a different way for the added test and from what I see in the generated baselines this resolves to @emotion/react JSX namespace correctly on the master (without my fix being applied yet). I suspect this is some kind of a configuration issue that makes this test behave differently than the provided repro but I don't know what it is. Any pointers for this would be appreciated.

As to the implementation - I've only applied this fix in one place and getJsxNamespace is used in some other places as well. One of them is even exposed as the public API of the compiler - I believe that it should account for implicit containers but they do not provide a string representation of the namespace (it's just an exported symbol/type from a particular module), so I'm not sure how this should be handled.

So in a sense, I feel that my patch is a bandaid right now because I've also found some other lines related to this resolution logic that looked really focused on the "classic" resolution and the "automatic" one seems a little bit tacked on to things. I believe this is out of the scope of this PR though, but maybe a candidate for a potential refactor later?

cc @weswigham

typescript-bot · 2020-11-10T14:44:32Z

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

Andarist · 2020-11-10T14:46:28Z

tests/cases/compiler/jsxNamespaceImplicitImportJSXNamespace2.tsx

@@ -0,0 +1,46 @@
+// @strict: true


this file should, of course, be renamed - but until I know more about the nature of the fix I'm not sure what it should be

weswigham · 2020-11-11T22:05:18Z

src/compiler/checker.ts

+                let resolvedNamespace: Symbol | undefined;
+                let namespaceName = getLocalClassicJsxNamespace(location);
+                if (!namespaceName) {
                    resolvedNamespace = getJsxNamespaceContainerForImplicitImport(location);
                }
+                if (!resolvedNamespace) {
+                    if (!namespaceName) {
+                        namespaceName = getConfigClassicJsxNamespace();
+                    }
+                    resolvedNamespace = resolveName(location, namespaceName, SymbolFlags.Namespace, /*diagnosticMessage*/ undefined, namespaceName, /*isUse*/ false);
+                }


Rather than globally changing the priority of the implicit lookup, we should instead just prefer the implicit namespace if jsx is react-jsx or react-jsxdev, no?

Suggested change

let resolvedNamespace: Symbol | undefined;

let namespaceName = getLocalClassicJsxNamespace(location);

if (!namespaceName) {

resolvedNamespace = getJsxNamespaceContainerForImplicitImport(location);

}

if (!resolvedNamespace) {

if (!namespaceName) {

namespaceName = getConfigClassicJsxNamespace();

}

resolvedNamespace = resolveName(location, namespaceName, SymbolFlags.Namespace, /*diagnosticMessage*/ undefined, namespaceName, /*isUse*/ false);

}

let resolvedNamespace: Symbol | undefined;

if (compilerOptions.jsx === JsxEmit.ReactJSX || compilerOptions.jsx === JsxEmit.ReactJSXDev) {

resolvedNamespace = getJsxNamespaceContainerForImplicitImport(location);

}

else {

const namespaceName = getJsxNamespace(location);

resolvedNamespace = resolveName(location, namespaceName, SymbolFlags.Namespace, /*diagnosticMessage*/ undefined, namespaceName, /*isUse*/ false);

}

If @jsxImportSource should be prioritized over @jsx then - yes. I also believe that this is a sensible priority order.

I can revert my changes quickly and apply the proposed patch but would prefer doing that after establishing how a failing test (for master) should look like so it would be verified by this PR that this actually solves the problem but - as I've mentioned - I have no idea right now that's the difference between my attempt at providing that failing test and the presented repro case. Any tips regarding that?

If @jsxImportSource should be prioritized over @jsx then - yes. I also believe that this is a sensible priority order.

Yeah, I think it should be. New thing > old thing is usually a good priority order.

Any tips regarding that?

The resolution behavior will change depending on what files are present "on disk" on the test. You may need to provide, eg, a node_modules/react/index.d.ts and/or a node_modules/react/jsx-runtime.d.ts (using minimal definitions for these, rather than the full react16 lib reference is likely what you'd need), or equivalent for an import source.

The resolution behavior will change depending on what files are present "on disk" on the test. You may need to provide, eg, a node_modules/react/index.d.ts and/or a node_modules/react/jsx-runtime.d.ts (using minimal definitions for these, rather than the full react16 lib reference is likely what you'd need), or equivalent for an import source.

I'm not sure if I completely understand this but this is definitely a good starting point for further exploration. It's already midnight here so I will get to it tomorrow and let you know if I've managed to create such a test.

@weswigham I've pushed out a good test case and a patch similar to what you have proposed. However, I've decided to call getJsxNamespaceContainerForImplicitImport unconditionally first because @jsxImportSource should be taken into account as well

Hm, alright. It looks like getJSXImplicitImportBase already matches that behavior, and that's what controls emit, so that seems reasonable.

Some tests of this would be good, though! So some tests where // @jsx: react but the file has a jsxImportSource pragma (indicating use of the new jsx transform).

Oh, so you are one of those... Added a new test to cover for this 😉

…red over config & pragma implicit ones

…g & pragma implicit ones

Andarist · 2020-11-13T14:57:54Z

...nes/reference/jsxNamespaceImplicitImportJSXNamespaceFromPragmaPickedOverGlobalOne.errors.txt

@@ -0,0 +1,69 @@
+/index.tsx(2,28): error TS2708: Cannot use namespace 'React' as a value.


I'm not sure how to fix this, nor if this is acceptable in this test case. Namespaces in TS are magic to me 😅

Drop something like an export class Component {} into the global react ns in the test.

Err, but actually I think this error indicates a place we're still looking at the default factory entity name, and not the import-source based one - might need to debug this to see where this error is being made from, and change it over to look inside the jsx runtime namespace instead.

So this error is coming from:
https://github.com/microsoft/TypeScript/blob/d72829757de7d496908e8ba52088e11f7c8613d4/src/compiler/checker.ts#L25422
and from what I understand this only used to prevent eliding jsx-related symbol from the emitted code, so I've wrapped that part of the code so it's now only executed when !getJsxNamespaceContainerForImplicitImport(node). Not sure if this is enough though - please re-check.

That looks pretty good. The emitter is responsible for adding the implicit import, so there's no need to even lookup the non-implicit one of we're not using it.

Andarist · 2020-11-14T10:29:28Z

...aselines/reference/jsxNamespaceImplicitImportJSXNamespaceFromPragmaPickedOverGlobalOne.types

@@ -3,7 +3,7 @@ export = React;
 >React : any

 export as namespace React;
->React : any
+>React : error


Hm, is this one expected? It also seems to be like this in the reactTransitiveImportHasValidDeclaration test which I haven't touched as part of this PR.

I wanna say "yes", since this location doesn't really have a type. It displays as any unless the test has no error diagnostics, in which case it's printed to the baselines as an error type to indicate a location we're incorrectly using the errorType instead of the anyType in the getTypeAtLocation API. Its not much of an issue in practice.

… runtime is used

Andarist · 2020-11-14T10:43:50Z

tests/baselines/reference/jsxEmptyExpressionNotCountedAsChild(jsx=react-jsx).js

@@ -21,8 +21,6 @@ const element = (
 "use strict";
 exports.__esModule = true;
 var jsx_runtime_1 = require("react/jsx-runtime");
-/// <reference path="react16.d.ts" />


from what I understand this actually got fixed because the default of importsNotUsedAsValues is remove and nothing from 'react' module is used here in the emitted output

Andarist · 2020-11-17T23:27:39Z

@weswigham friendly 🏓 I wouldn't normally ping a maintainer like this but it would be lovely if we could land this before the final 4.1 release and from what I understand this is about to be released in 2 days.

weswigham · 2020-11-18T13:59:03Z

@typescript-bot cherrypick this into release-4.1

typescript-bot · 2020-11-18T13:59:26Z

Heya @weswigham, I've started to run the task to cherry-pick this into release-4.1 on this PR at 9e18902. You can monitor the build here.

typescript-bot · 2020-11-18T14:03:17Z

Hey @weswigham, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-4.1 manually.

* Add actual baselines for a problem with global namespace being preferred over config & pragma implicit ones * Fixed an issue with global React namespace being preferred over config & pragma implicit ones * Do not try to mark JSX classic runtime symbols as used when automatic runtime is used

* Add actual baselines for a problem with global namespace being preferred over config & pragma implicit ones * Fixed an issue with global React namespace being preferred over config & pragma implicit ones * Do not try to mark JSX classic runtime symbols as used when automatic runtime is used Co-authored-by: Mateusz Burzyński <mateuszburzynski@gmail.com>

typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 10, 2020

Andarist commented Nov 10, 2020

View reviewed changes

Andarist mentioned this pull request Nov 10, 2020

Auto-include types for the jsx import source in the new jsx transforms #41330

Merged

DanielRosenwasser requested a review from weswigham November 11, 2020 21:07

weswigham reviewed Nov 11, 2020

View reviewed changes

Andarist force-pushed the fix/jsx-global-preferred-over-config-implicit branch 3 times, most recently from e5d648c to 9787509 Compare November 13, 2020 14:52

Andarist added 2 commits November 13, 2020 15:56

Add actual baselines for a problem with global namespace being prefer…

f46544d

…red over config & pragma implicit ones

Fixed an issue with global React namespace being preferred over confi…

0870655

…g & pragma implicit ones

Andarist force-pushed the fix/jsx-global-preferred-over-config-implicit branch from 9787509 to 0870655 Compare November 13, 2020 14:56

Andarist commented Nov 13, 2020

View reviewed changes

Andarist commented Nov 14, 2020

View reviewed changes

Do not try to mark JSX classic runtime symbols as used when automatic…

9e18902

… runtime is used

Andarist force-pushed the fix/jsx-global-preferred-over-config-implicit branch from e772e51 to 9e18902 Compare November 14, 2020 10:40

Andarist commented Nov 14, 2020

View reviewed changes

Andarist mentioned this pull request Nov 14, 2020

Make JSX pragmas override the configured runtime mode babel/babel#12208

Open

1 task

weswigham approved these changes Nov 18, 2020

View reviewed changes

weswigham merged commit 09eb3d3 into microsoft:master Nov 18, 2020

Andarist deleted the fix/jsx-global-preferred-over-config-implicit branch November 18, 2020 08:37

This was referenced Nov 20, 2020

Add emit support for jsx/jsxs experimental jsx runtime api #39199

Merged

Add a test for JSX namespace lookup with jsx: preserve, jsxImportSource #41676

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/jsx global preferred over config implicit #41476

Fix/jsx global preferred over config implicit #41476

Andarist commented Nov 10, 2020 •

edited

Loading

typescript-bot commented Nov 10, 2020

Andarist Nov 10, 2020

weswigham Nov 11, 2020

Andarist Nov 11, 2020

weswigham Nov 11, 2020

Andarist Nov 11, 2020

Andarist Nov 12, 2020

weswigham Nov 13, 2020

weswigham Nov 13, 2020

Andarist Nov 13, 2020

Andarist Nov 13, 2020

weswigham Nov 13, 2020

weswigham Nov 13, 2020

Andarist Nov 13, 2020

Andarist Nov 14, 2020

weswigham Nov 18, 2020

Andarist Nov 14, 2020

weswigham Nov 18, 2020

Andarist Nov 14, 2020

Andarist commented Nov 17, 2020

weswigham commented Nov 18, 2020

typescript-bot commented Nov 18, 2020 •

edited

Loading

typescript-bot commented Nov 18, 2020

		@@ -0,0 +1,69 @@
		/index.tsx(2,28): error TS2708: Cannot use namespace 'React' as a value.

Fix/jsx global preferred over config implicit #41476

Fix/jsx global preferred over config implicit #41476

Conversation

Andarist commented Nov 10, 2020 • edited Loading

typescript-bot commented Nov 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarist commented Nov 17, 2020

weswigham commented Nov 18, 2020

typescript-bot commented Nov 18, 2020 • edited Loading

typescript-bot commented Nov 18, 2020

Andarist commented Nov 10, 2020 •

edited

Loading

typescript-bot commented Nov 18, 2020 •

edited

Loading