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

JSX .tsx support #15

Closed
fregante opened this issue Mar 14, 2019 · 18 comments
Closed

JSX .tsx support #15

fregante opened this issue Mar 14, 2019 · 18 comments

Comments

@fregante
Copy link

fregante commented Mar 14, 2019

// index.test-d.tsx
import {expectType} from 'tsd-check';
import React from 'dom-chef';

expectType<HTMLDivElement>(<div/>);
@SamVerschueren
Copy link
Collaborator

I'm really not familiar with .tsx. Is the only thing we should do is check if it's a index.test-d.tsx or a index.test-d.ts is present?

A failing test for this would be nice as would allow me to fix it more easily and make sure I don't misunderstand the problem :).

@sindresorhus
Copy link
Collaborator

Is the only thing we should do is check if it's a index.test-d.tsx or a index.test-d.ts is present?

Correct:

In order to use JSX you must do two things.

  1. Name your files with a .tsx extension
  2. Enable the jsx option

- https://www.typescriptlang.org/docs/handbook/jsx.html

@fregante
Copy link
Author

fregante commented Mar 20, 2019

Also, disregard my first example because dom-chef types aren't there yet.

This should be your test, I think:

// index.test-d.tsx
import {expectType} from 'tsd-check';
import React from 'react';

class Welcome extends React.Component() {
    constructor() {
        super();
        return <h1>Hello</h1>;
    }
}

expectType<Welcome>(<Welcome/>);

And this would be the JSX-less equivalent example, which I just tested and works:

// index.test-d.ts
import {expectType} from 'tsd-check';
import React from 'react';

class Welcome extends React.Component() {
    constructor() {
        super();
        return React.createElement("h1", null, "Hello");
    }
}
expectType<Welcome>(React.createElement(Welcome, null));

@sindresorhus
Copy link
Collaborator

It's really stupid that we're forced to use a .tsx extension, but that's how it is until microsoft/TypeScript#30503 is resolved.

@SamVerschueren
Copy link
Collaborator

Thanks @bfred-it for the test case! I'll see what I can do.

@SamVerschueren
Copy link
Collaborator

I tried looking into this, but not sure if I understand the problem correctly.

The thing is that the expectType in your example will never throw.

class Welcome extends React.Component() {
	constructor() {
		super();
		return <h1>Hello</h1>;
	}
}

class Unicorn extends React.Component() {
	constructor() {
		super();
		return <h2>Unicorn</h2>;
	}
}

expectType<Welcome>(<Unicorn/>);

The reason is that <Unicorn /> is of type any in TypeScript.

So my question actually is, why do we need tsd to pick up test-d.tsx files? For example, if I look at the typings of dom-chef, I don't see a reason why the test for that type definition should have a tsx file extension? What kind of tsd test could be written that it has to be .tsx?

I'm just wondering and clarifying so that we are all on the same page :). I could totally add it for convenience, but I want to know the 'why'.

@fregante
Copy link
Author

fregante commented Jul 7, 2019

If you enable JSX in TypeScript the type should be JSX.Element

@SamVerschueren
Copy link
Collaborator

This is what I get

image

Even a simple <div /> element doesn't work...

image

I must be missing something I guess :thinking_face:

@SamVerschueren
Copy link
Collaborator

On a sidenote, I installed @types/react. Is that necessary?

@fregante
Copy link
Author

Yeah, JSX types are awful, The type 'Element' it's referring to is not the browser's Element but JSX.Element specifically. So that should probably be:

expectType<JSX.Element>(<div/>);

I made a mistake in my OP

@fregante
Copy link
Author

fregante commented Sep 16, 2019

Even in that case I'm not entirely sure that <div/> will work. In Refined GitHub we also have this:

https://github.com/sindresorhus/refined-github/blob/6631e5f351781e714e6eb3c6af24d76a6af301e7/source/globals.d.ts#L40-L52

As for the Unicorn type... no idea honestly 😬

@SamVerschueren
Copy link
Collaborator

SamVerschueren commented Sep 16, 2019

But what to do in case of Unicorn? We can't really type check that, right?

class Unicorn extends React.Component {
	constructor(props) {
		super(props);
	}

	render() {
		return <h1>Unicorn</h1>;
	}
}

expectType<Unicorn>(<Unicorn />);

Or would it be enough that tsd just picks up tsx files? If that's the case, I think it works right now (on my machine :p).

Checking it against JSX.Element works, but that's not really useful, or is it?

expectType<JSX.Element>(<Unicorn />);

@fregante
Copy link
Author

fregante commented Sep 16, 2019

I think technically <Unicorn /> isn't of type Unicorn but what React.createElement(Unicorn) maps to:

React.createElement(Unicorn);
// appears to be of type
React.ComponentElement<{}, Unicorn>

Also:

React.createElement('div');
// appears to be of type
React.DetailedReactHTMLElement<React.HTMLAttributes<HTMLElement>, HTMLElement>`

@SamVerschueren
Copy link
Collaborator

For some reason, this doesn't fail.

expectType<React.ComponentElement<{}, Welcome>>(<Unicorn />);
expectType<React.ComponentElement<{}, Welcome>>(React.createElement(Unicorn));

Let's just enable this feature and see how it goes. tsd can't help it if the JSX or React type definitions are bad. As long as it picks up the tsx/jsx files, we're good to go. It's really a small change in the codebase.

SamVerschueren added a commit that referenced this issue Sep 17, 2019
@SamVerschueren
Copy link
Collaborator

I could use some help testing #36 to see if this is what is expected :).

@SamVerschueren
Copy link
Collaborator

Released as 0.8.0

@fregante
Copy link
Author

Thank you ☺️

@SamVerschueren
Copy link
Collaborator

Thank you for opening the issue and helping me understand the problem :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants