Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incomplete type cleanup with jsx set to preserve #37456

Closed
Vayner opened this issue Mar 18, 2020 · 9 comments · Fixed by #37739
Closed

Incomplete type cleanup with jsx set to preserve #37456

Vayner opened this issue Mar 18, 2020 · 9 comments · Fixed by #37739
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this

Comments

@Vayner
Copy link

Vayner commented Mar 18, 2020

TypeScript Version: 3.8.3

Search Terms:

  • jsx generic
  • jsx unknown
  • jsx generic unknown

Expected behavior:
No types in output when jsx is set to preserve.

Actual behavior:
Some types in output, meaning the resulting JSX is invalid.

Related Issues:

Code

import * as React from 'react';

class Test<P> extends React.Component<P> {
        render() {
            return "Hello"
        }
}

type SomeType = {
    foo?: boolean
}

interface SomeInterface { 
    bar?: boolean
}

const usingTest: React.FC = () => {
    <>
        <Test<{}> />
        <Test<SomeType> />
        <Test<SomeInterface> />
        <Test<unknown> />
        <Test<string> />
        <Test<boolean> />
        <Test<object> />
        <Test<null> />
        <Test<any> />
        <Test<never> />
    </>
}

Output

import * as React from 'react';
class Test extends React.Component {
    render() {
        return "Hello";
    }
}
const usingTest = () => {
    <>
        <Test />
        <Test />
        <Test />
        <Test<unknown> />
        <Test />
        <Test />
        <Test<object> />
        <Test<null> />
        <Test />
        <Test />
    </>;
};
Compiler Options
{
  "compilerOptions": {
    "noImplicitAny": true,
    "strictNullChecks": true,
    "strictFunctionTypes": true,
    "strictPropertyInitialization": true,
    "strictBindCallApply": true,
    "noImplicitThis": true,
    "noImplicitReturns": true,
    "useDefineForClassFields": false,
    "alwaysStrict": true,
    "allowUnreachableCode": false,
    "allowUnusedLabels": false,
    "downlevelIteration": false,
    "noEmitHelpers": false,
    "noLib": false,
    "noStrictGenericChecks": false,
    "noUnusedLocals": false,
    "noUnusedParameters": false,
    "esModuleInterop": true,
    "preserveConstEnums": false,
    "removeComments": false,
    "skipLibCheck": false,
    "checkJs": false,
    "allowJs": false,
    "declaration": true,
    "experimentalDecorators": false,
    "emitDecoratorMetadata": false,
    "target": "ES2017",
    "module": "ESNext",
    "jsx": "preserve"
  }
}

Playground Link: Provided

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Mar 18, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.9.1 milestone Mar 18, 2020
@DanielRosenwasser DanielRosenwasser added Good First Issue Well scoped, documented and has the green light Help Wanted You can do this labels Mar 23, 2020
@Davenporten
Copy link

I'm pretty new to TS and this is the first time I'm trying to do anything with the compiler, but I'll take a crack at this one if you don't think it's beyond a noob.

@Davenporten
Copy link

Davenporten commented Mar 24, 2020

I get a much more complex output than the one above. After running the command node ..\built\local\tsc.js --jsx preserve .\testReact.tsx I get

"use strict";
var __extends = (this && this.__extends) || (function () {
    var extendStatics = function (d, b) {
        extendStatics = Object.setPrototypeOf ||
            ({ __proto__: [] } instanceof Array && function (d, b) { d.__proto__ = b; }) ||
            function (d, b) { for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p]; };
        return extendStatics(d, b);
    };
    return function (d, b) {
        extendStatics(d, b);
        function __() { this.constructor = d; }
        d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
    };
})();
exports.__esModule = true;
var React = require("react");
var Test = /** @class */ (function (_super) {
    __extends(Test, _super);
    function Test() {
        return _super !== null && _super.apply(this, arguments) || this;
    }
    Test.prototype.render = function () {
        return "Hello";
    };
    return Test;
}(React.Component));
var usingTest = function () {
    <>
        <Test />
        <Test />
        <Test />
        <Test<unknown> />
        <Test />
        <Test />
        <Test<object> />
        <Test<null> />
        <Test />
        <Test />
    </>;
};

Is this expected? I just want to make sure I'm not missing any other configs other than the --jsx preserve one.

@hikarino-my
Copy link
Contributor

hikarino-my commented Mar 24, 2020

@Davenporten
That is expected emit with that config.
If you want clean output, give these configs.

--target es2017
--jsx preserve
--moduleResolution Node

Also this sample code will be better for debug since this doesn't emit error on types.

import * as React from "react";
class Test<P> extends React.Component {
  render() {
    return "Hello";
  }
}
type SomeType = {
  foo?: boolean;
};
interface SomeInterface {
  bar?: boolean;
}
const usingTest: React.FC = () => (
  <>
    <Test<{}> />
    <Test<SomeType> />
    <Test<SomeInterface> />
    <Test<unknown> />
    <Test<string> />
    <Test<boolean> />
    <Test<object> />
    <Test<null> />
    <Test<any> />
    <Test<never> />
  </>
);

@orta
Copy link
Contributor

orta commented Mar 24, 2020

@davenport - you can see the compiler flags enabled in the playground link (the key one here is Module: ESNext)

(heh, looks like @hikarinotomadoi beat me to it)

@a-tarasyuk
Copy link
Contributor

I've found two PRs that are related to this issue. In the original PR, I found two comments #22415 (comment), #22415 (comment)

Question: When tsconfig is set to "tsx": "preserve", is the generic removed?
It aught to be. If not, it's a bug.

It seems typeArguments shouldn't be emitted for JSX Elements. Also, there is PR where typeArguments emitter was added for JSX Elements., this PR hasn't any tests, and there is no linked issue to understand which problem it fixes.

@weswigham Is there any case where typeArguments should be emitted?

cc @rbuckton @DanielRosenwasser @orta

@weswigham
Copy link
Member

The emitter pretty prints TS, too, so it should be able to print all nodes. The file transforms/ts.ts is supposed to visit TS-only nodes (like these) and elide them from the tree before emit. That it's dependent on the type in the type arguments makes me think it's a bug in transform flags aggregation, though. (Namely, those type nodes failing to mark the containing node as containing TS)

@a-tarasyuk
Copy link
Contributor

a-tarasyuk commented Apr 1, 2020

That it's dependent on the type in the type arguments makes me think it's a bug in transform flags aggregation, though.

I think changing transform flags aggregation can fix cases with unknown and object types, however, for type arguments that use null, undefined needs another way. I'm not sure, maybe JSX elements nodes should be updated in transforms/ts.ts to eliminate typeArguments, like it does for call expressions 😕. What do you @weswigham think?

@weswigham
Copy link
Member

Probably! Call expression type arguments and jsx expression type arguments are essentially similar, so transforming them in the same way makes sense.

@DanielRosenwasser
Copy link
Member

Thanks @a-tarasyuk!

@DanielRosenwasser DanielRosenwasser added the Fixed A PR has been merged for this issue label Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue Good First Issue Well scoped, documented and has the green light Help Wanted You can do this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants