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

TS editor inference and autocomplete doesn't seem to work. #2775

Closed
bryceosterhaus opened this issue Dec 6, 2019 · 16 comments · Fixed by #2816
Closed

TS editor inference and autocomplete doesn't seem to work. #2775

bryceosterhaus opened this issue Dec 6, 2019 · 16 comments · Fixed by #2816
Labels
3.x comp: clay-components Issues related to Clay components. (e.g ClayCard, ClayLabel...) type: bug Issues reporting that Component is not doing what should be done typescript

Comments

@bryceosterhaus
Copy link
Member

Check out the example below. The antd library correctly allows users to autocomplete its value for type. The ClayButton does not work for autocompleting values. Note this is in the editor, not related to the UI.

https://codesandbox.io/s/sad-panini-o3oez

Tech Version
Clay v3.x
@bryceosterhaus bryceosterhaus added type: bug Issues reporting that Component is not doing what should be done comp: clay-components Issues related to Clay components. (e.g ClayCard, ClayLabel...) 3.x typescript labels Dec 6, 2019
@bryceosterhaus bryceosterhaus self-assigned this Dec 12, 2019
@bryceosterhaus
Copy link
Member Author

It seems to work correctly if you add "esModuleInterop": true, to your tsconfig

@bryceosterhaus
Copy link
Member Author

similar reported issue microsoft/TypeScript#28009

@bryceosterhaus
Copy link
Member Author

I think I've hit a bit of a dead end here.

A few possible solutions:

  1. Refactor our use of named default exports React from 'react' and instead use start syntax of * as React from 'react'.
  2. Don't worry about it and just tell people to use "esModuleInterop": true for proper editor and autocomplete support.

Any other ideas for this or have anyone ran into this? cc @matuzalemsteles @wincent

@julien
Copy link
Contributor

julien commented Dec 13, 2019

It seems to me @andresfulla reported something about autocomplete in #2235 not 100% sure if it's related though.

@wincent
Copy link
Contributor

wincent commented Dec 16, 2019

Using import * as React from 'react' doesn't seem like the worst thing in the world, or rather, it seems better than forcing people to use a particular esModuleInterop setting (they may have good reasons for not wanting to). If you think that will fix it without any downsides other than requiring us to go back to something that we had before, then I'd say go for it.

FWIW, I think the appeal of avoiding importing the entire namespace may be more theoretical than practical: even if you do tree-shaking, real apps end up using just about all of React somewhere, so treating it as a monolithic bundle is probably not as bad as it sounds.

@wincent
Copy link
Contributor

wincent commented Dec 16, 2019

Fun fact: I remember talking about this back in February so I went back to see if anything had changed about the way React declares its exports. It's still the same (React master, still b87aabd, ReactDOM master, still b87aabd) and I guess will be for the foreseeable future.

@matuzalemsteles
Copy link
Member

Hmm that's weird, why would using import * as React from 'react' solve the problem? just for curiosity.

@bryceosterhaus
Copy link
Member Author

By switching to use * as ... it would allow us to remove the esModuleInterop config, meaning other projects consuming us wouldn't require that.

I am going to test a different route and see if I can just get away with using allowSyntheticDefaultImports instead though.

@bryceosterhaus
Copy link
Member Author

Essentially the type definitions are being generated with React from 'react'. For example, with our current config, this is the definition for Button.

import React from 'react';
export declare type DisplayType = 'primary' | 'secondary' | 'link' | 'unstyled';
interface IProps extends React.ButtonHTMLAttributes<HTMLButtonElement> {
    borderless?: boolean;
    block?: boolean;
    displayType?: DisplayType;
    monospaced?: boolean;
    outline?: boolean;
    small?: boolean;
}
declare const _default: React.ForwardRefExoticComponent<IProps & React.RefAttributes<HTMLButtonElement>> & {
    Group: React.FunctionComponent<import("./Group").IButtonGroupProps>;
};
export default _default;

When this definition gets loaded into a project without esModuleInterop, it errors on the react import, which then defaults the exported type to any. Therefore, when we consume Button, it shows its an any type.

If I remove esModuleInterop and then in the src file change it to use * as ... syntax. I get the following definition file.

import * as React from 'react';
export declare type DisplayType = 'primary' | 'secondary' | 'link' | 'unstyled';
interface IProps extends React.ButtonHTMLAttributes<HTMLButtonElement> {
    borderless?: boolean;
    block?: boolean;
    displayType?: DisplayType;
    monospaced?: boolean;
    outline?: boolean;
    small?: boolean;
}
declare const _default: React.ForwardRefExoticComponent<IProps & React.RefAttributes<HTMLButtonElement>> & {
    Group: React.FunctionComponent<import("./Group").IButtonGroupProps>;
};
export default _default;

This then works correctly in a project consuming Clay without having to set esModuleInterop to true.

The only different is the * as ... syntax in the definition file.

@bryceosterhaus
Copy link
Member Author

I believe this PR should fix this issue. Its just a matter of discussion if we want to actually go this route of removing esModuleInterop. #2816

Note: the CSB build doesnt pick up types, but I think that is an issue with their CI. I tried to build and use the package locally and it worked fine.

@wincent
Copy link
Contributor

wincent commented Dec 17, 2019

When this definition gets loaded into a project without esModuleInterop, it errors on the react import, which then defaults the exported type to any. Therefore, when we consume Button, it shows its an any type.

Excellent detective work! 🕵

@matuzalemsteles
Copy link
Member

Hmm, this is still strange to me, why am I wondering why the editor is not getting the types from @types/react ... but I have no problem changing to * as ... just for the React, I think like @wincent said, the problem would be just tree-shaking I'm not sure if it would be a problem for Portal, probably...

@bryceosterhaus
Copy link
Member Author

@matuzalemsteles I don't think we would have any difference for tree shaking purposes, plus im not even sure we utilize and tree shaking at the moment. One thing to note is that we would have to do this for all packages that we normally just take the default import. See this pr, #2816, for changes required.

@matuzalemsteles
Copy link
Member

@bryceosterhaus Yeah, no problem for me.

@bryceosterhaus
Copy link
Member Author

reopening because of #2851

@bryceosterhaus bryceosterhaus removed their assignment Mar 13, 2020
@matuzalemsteles
Copy link
Member

I'm closing this because this doesn't seem to be a problem for the developer anymore, we also added an improvement in the types to provide the descriptions of the props recently #4836. In case this is still an issue for the Storybook environment, jest, and development I will open this again or create a new issue to keep it on top.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x comp: clay-components Issues related to Clay components. (e.g ClayCard, ClayLabel...) type: bug Issues reporting that Component is not doing what should be done typescript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants