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

Changes declaration to true. #4403

Closed
wants to merge 1 commit into from
Closed

Conversation

MicahZoltu
Copy link

In order for this library to be usable in a TypeScript project, declaration files need to be generated. This is trivially easy for a TS project, you just need to set this flag to true and they will be generated automatically on compile. No idea why this wasn't set before, but it should basically always be set for libraries.

In order for this library to be usable in a TypeScript project, declaration files need to be generated.  This is trivially easy for a TS project, you just need to set this flag to true and they will be generated automatically on compile.  No idea why this wasn't set before, but it should basically always be set for libraries.
@MicahZoltu
Copy link
Author

This is a much less aggressive dupe of #4375. #4375 tries to emit for JS files and tries to fix this issue across all packages. This PR, on the other hand, only generates .d.ts files for react-admin package which has been fully migrated to TS already and does not have any JS files.

Recommend merging this PR ASAP, and iterate on #4375 until it is ready. Also, probably worth considering breaking #4375 up into multiple smaller PRs, it feels like a pretty big undertaking all at once. For example, you could just enable declaration file emission in projects that are fully migrated, one project per PR at a time.

@MicahZoltu
Copy link
Author

Also just saw #4115, where someone claims that this change will "break all projects using TypeScript". I would like to see a much deeper explanation why this is believed to be true, because on the surface that claim suggests a bug in TypeScript!

At the moment, any TS project using react-admin doesn't get any type checking unless they hand roll them in their own .d.ts file within their project, which is almost certainly going to be worse than the auto-generated one by the compiler. Any project that is "broken" by the enabling of this flag was already broken previously, and the only change is that they will now get a compile time error instead of a runtime error.

@fzaninotto
Copy link
Member

The same comment as in #4375 applies here:

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.

This is because the types present in the code were never tested in a real app. Testing them on a real world app (like the demo example) is necessary before publishing them, because I'm 100% sure that some of these types are wrong (e.g. required prop that is actually optional, but that prevents the TypeScript compilation).

If you want to speed up TypeScript in react-admin, please focus on migrating the demo example.

@MicahZoltu
Copy link
Author

@fzaninotto I don't actually use this package, so don't care enough to work on migrating the demo app. Someone came into the TypeScript channel asking for help and seeing a TS project that doesn't ship .d.ts files is a huge red flag (this is almost always a mistake).

Is the issue here that you are migrating a JS project to a TS project from the outside in, which means everything at the core is just (essentially) any and so your outer types are really just best guesses as to what the inner types should be? If so, my general recommendation is to go the other direction (inside out), so the compiler can do most of the work for you.

@fzaninotto
Copy link
Member

Due to the use of very dynamic JS features, children cloning, and props injection, what you describe isn't feasible with our code base.

We need an example app to test the types before publishing them.

@Acerbic
Copy link

Acerbic commented Jun 20, 2020

Alternatively, you could add

"typings": "src/index.ts",

to the react-admin's package.json.

I did it and my VSCode stopped complaining.

@fzaninotto
Copy link
Member

superseded by #5291

@fzaninotto fzaninotto closed this Oct 1, 2020
@MicahZoltu MicahZoltu deleted the patch-1 branch October 1, 2020 11:13
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.

3 participants