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 types are generated with unsafe types because strictNullChecks is false #9622

Closed
gauthierm opened this issue Jan 29, 2024 · 5 comments

Comments

@gauthierm
Copy link

What you were expecting:

With this code the type of record should be MyRecord | undefined:

const record = useRecordContext<MyRecord>();

What happened instead:

The type of record is MyRecord.

Steps to reproduce:

  1. Install react-admin in a TypeScript project
  2. Write some code that uses useRecordContext
  3. Use the record without checking for undefined because the return type says it cannot be undefined
  4. Get a runtime error.

Other information:

This issue is caused by the tsconfig.json in the base react-admin project not using strict mode, specifically strictNullChecks. This means TypeScript will coalesce a type like RaRecord | undefined into RaRecord. See https://www.typescriptlang.org/tsconfig#strictNullChecks

A previous bug was recorded #7588 and fixed in PR #7498. The fix is correct if strictNullChecks is used, but because TypeScript is not configured to consider undefined, the fix doesn't actually do anything.

To confirm this, you can check the build output in either the esm or cjs folders. The | undefined is omitted from the generated types.

Environment

  • React-admin version: 4.16.7
  • Last version that did not exhibit the issue (if applicable): n/a
  • React version: 18.x
  • Browser: n/a
  • Stack trace (in case of a JS error): n/a
@fzaninotto
Copy link
Member

Hi, and thanks for your report.

We're well aware of the limitations caused by structNullChecks=false in the quality of types. We have it on our radar, but the benefit / cost ratio of this task is low, so it's not a priority for us. Fixing it is non-trivial, as we have to make ra-core compilable with strict?NullChecks=true. Previous attempts (#8141) lead to a dead end.

You're welcome to contribute small PRs fixing types of individual hooks & components for strict null checks.

I'm closing this issue as it's a long-term goal.

@gauthierm
Copy link
Author

Thanks for letting us know it's a long term goal. It's not possible to fix the issue with the hooks until strictNullChecks is enabled. It's currently not possible to use the generated types safely in projects.

@gauthierm
Copy link
Author

@fzaninotto What sort of approach would be required to get strictNullChecks enabled? It looks like someone tried to help you with it in #8141, but because the scope of the change was too big the PR was closed.

I'm not sure how you could partially add strict null checks to make for a smaller PR that would be accepted.

The benefit is quite high for people using React Admin as it will catch a lot of runtime errors at compile time.

@fzaninotto
Copy link
Member

Here is the process I suggest:

  1. Turn on strickNullChecks in your checkout
  2. Try to compile the core
  3. Choose a part of the library (auth, forms, etc)
  4. Fix a reasonable amount of errors to make the change readable
  5. Turn off strickNullChecks
  6. Open a PR with your changes.

This way, we can gradually make the types better. It will take several PRs to fix the core. This improvement will only be made available for users once we have all the checks fixed. But that's the only safe way.

@fzaninotto
Copy link
Member

Here is an exemple small PR fixing the ra-core types: #9644

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

2 participants