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

typescript declarations emit #4375

Closed
wants to merge 2 commits into from

Conversation

Bnaya
Copy link
Contributor

@Bnaya Bnaya commented Jan 31, 2020

Emit typescript declarations - utilize typescript 3.7 ability to emit types for js code.
Branched out from #3805

Minor code changes to make the types compilation pass

@Bnaya Bnaya requested a review from fzaninotto January 31, 2020 21:40
Module 'ra-core' has already exported a member named 'Notification'. Consider explicitly re-exporting to resolve the ambiguity.ts(2308)
Module 'ra-core' has already exported a member named 'Pagination'. Consider explicitly re-exporting to resolve the ambiguity.ts(2308)
*/
// @ts-ignore
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fzaninotto what's the correct symbols here should be?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe ra-ui-material-ui should import those types from ra-core

@user753
Copy link

user753 commented Feb 3, 2020

There is many required props like
classes required

declare function Datagrid({ classes: classesOverride, ...props }: {
    [x: string]: any;
    classes: any;
}): JSX.Element;

record required

declare function ReferenceField({ children, record, source, ...props }: {
    [x: string]: any;
    children: any;
    record: any;
    source: any;
}): JSX.Element;

ReferenceInput
all required

interface Props {
    allowEmpty: boolean;
    basePath: string;
    children: ReactElement;
    classes: any;
    className: string;
    label: string;
    reference: string;
    resource: string;
    [key: string]: any;
}

@Bnaya
Copy link
Contributor Author

Bnaya commented Feb 3, 2020

@user753 this is by no-mean full typescript coverage.
The types from the js files are based on the guesses of TSC.
But it is a great start for symbol discovery and a way to make incremental improvements to the types,
via tsx refactor or jsdocs-types in exiting js files

@user753
Copy link

user753 commented Feb 3, 2020

@Bnaya I understand that. I think It could be fixed if all guessed props were optional.
Maybe there is a simple way to do that?
Also if these types was on https://github.com/DefinitelyTyped/DefinitelyTyped we could easily fix them.

Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please PR on next, as this isn't a bug fix.

@@ -33,6 +33,9 @@ const useStyles = makeStyles(
{ name: 'RaLoading' }
);

/**
* @type {React.FunctionComponent<{className: string; classes: object; loadingPrimary?: string; loadingSecondary?: string;}>}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer that you turn that component to TypeScript. I don't want to maintain a lib with 3 different syntax (JS, TS, and JS-with-types)

Module 'ra-core' has already exported a member named 'Notification'. Consider explicitly re-exporting to resolve the ambiguity.ts(2308)
Module 'ra-core' has already exported a member named 'Pagination'. Consider explicitly re-exporting to resolve the ambiguity.ts(2308)
*/
// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe ra-ui-material-ui should import those types from ra-core

@fzaninotto
Copy link
Member

Also, before emitting the type definitions from ra-ui-material-ui, we must migrate the demo to TypeScript, so that we can make sure the types don't break existing apps. Would you like to tackle that task first?

@Bnaya Bnaya changed the base branch from master to next February 3, 2020 16:17
@Bnaya
Copy link
Contributor Author

Bnaya commented Feb 3, 2020

migrate the demo ...Would you like to tackle that task first

I've enabled checkJs for the demo and ill go over the errors i'm getting, That's should be equivalent to typescript refactor +-
I've also deleted the examples/demo/src/react-admin.d.ts file, and now my VSCODE picking up the types built from my workspace.

In general, i have the feeling the component composition api RA works with not playing well with strict typescript. for example:
Screen Shot 2020-02-03 at 18 32 11
That's a valid RA code, but there is no way to tell typescript that "if your parent component has source prop you don't need one of your own"
So i'm not sure to where to take it from here

that's because we want to be sure that the types we publish don't break existing TS apps

I think it will be very difficult to guarantee that no typescript app will not break in any way after publishing types with react-admin
each project has it's own hacks around types now
For me it's @ts-ignore all over the imports, for others its empty react admin d.ts
Even if our new types would be perfect, it might collide with any d.ts that in the consuming projects

@fzaninotto
Copy link
Member

On the example you underline, if the source can be injected, in that case it should not be required in the types.

@Bnaya Bnaya force-pushed the typescript-declarations-emit branch from e293c88 to 8bf3480 Compare February 4, 2020 08:38
@Bnaya
Copy link
Contributor Author

Bnaya commented Feb 4, 2020

I'm willing to put effort to make that happen, but there are quite few things that are not clear to me
I would like to talk directly if possible to find the best course of action

@Vadorequest
Copy link

@Bnaya Check this out #1617 (comment) :)

@fzaninotto
Copy link
Member

@Bnaya What isn't clear and what do you want to talk about?

@matepaiva
Copy link

I am really looking forward to this feature! It's really helpful!

@fzaninotto fzaninotto closed this Jun 5, 2020
@falkenhawk
Copy link

@fzaninotto since you closed this, I guess this is now superceded by #4505 ?

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

Successfully merging this pull request may close these issues.

6 participants