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

Use "strictNullChecks": true in ra-core #8141

Closed
wants to merge 3 commits into from

Conversation

sixers
Copy link

@sixers sixers commented Sep 5, 2022

This PR enables strictNullChecks mode in ra-core, which is required by many useful TypeScript packages (e.g. https://github.com/colinhacks/zod or https://trpc.io/)

The current implementation is just bare minimum to make it compile, and has a handful of ugly type casts. I submitted it to open a a discussion on the best approach to resolve issues - there are a few alternatives.

I added some comments which explain the issues and lists possible solutions.

@@ -32,7 +32,7 @@ const CategoryList = () => (

const CategoryGrid = () => {
const { data, isLoading } = useListContext<Category>();
if (isLoading) {
if (isLoading || !data) {
Copy link
Author

Choose a reason for hiding this comment

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

This is the controversial part I guess, because the current ListControllerResult types are incorrect, and data can actually be undefined. I tried to make ListControllerResult conditional, but unfortunately it's not sufficient to just condition on isLoading. E.g. query could be disabled and idle or return error, and have both data: undefined and isLoading: false.

Therefore, the easiest option was to just make the data optional, which unfortunately means that I had to augment demo code with additional checks.

@@ -45,21 +44,20 @@ const useGetIdentity = () => {
const authProvider = useAuthProvider();
useEffect(() => {
if (authProvider && typeof authProvider.getIdentity === 'function') {
const callAuthProvider = async () => {
try {
const identity = await authProvider.getIdentity();
Copy link
Author

@sixers sixers Sep 5, 2022

Choose a reason for hiding this comment

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

This complained about authProvider.getIdentity possibly being undefined. There is a check, but it's in outer scope, and this is async function which can be executed later, so this check doesn't apply to the inner function. Therefore, I changed it to call authProvider.getIdentity() immediately in the current scope and simply use .then() / .catch() to store the results.

@@ -91,7 +91,7 @@ export const useCreate = <
meta: callTimeMeta = paramsRef.current.meta,
} = {}) =>
dataProvider
.create<RecordType>(callTimeResource, {
.create<RecordType>(callTimeResource as string, {
Copy link
Author

Choose a reason for hiding this comment

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

This is a bit of a hack. I assume the intention is that you either have to provide resource in useCreate() invocation, or you need to pass it to returned mutation function:

const [create] = useCreate('product')
const doCreateProduct = useCallback((data) => create({data}));
const [create] = useCreate();
const doCreateProduct = useCallback((data) => create({resource: 'product', data}));

Only these 2 cases should be allowed. If that's the intention, then I see 2 options:

  1. Make UseCreateResult result conditional based on the provided resource, so that it's required param in the returned mutation function if it's undefined, or optional if it's not undefined
  2. Just do a runtime check and don't call dataProvider if callTimeResource is undefined.

WDYT?

.delete<RecordType>(callTimeResource, {
id: callTimeId,
.delete<RecordType>(callTimeResource as string, {
id: callTimeId as Identifier,
Copy link
Author

Choose a reason for hiding this comment

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

Similar to the hack above, and we have similar options: 1) callTimeId should only be optional if params.id is not undefined, and required otherwise 2) just do a runtime check

@slax57
Copy link
Contributor

slax57 commented Sep 6, 2022

This is really a massive work you started here!
This was not in our top priorities at the moment, so I don't know when we will have time to review and discuss about your PR, but it will certainly help that you started to tackle the work this far, and I definitely appreciate the initiative!
Thanks!

@fzaninotto
Copy link
Member

I can foresee this will be very hard to review - I've seen nasty impacts with that type of changes in the past. I'd prefer if you split it into several smaller PRs.

@sixers
Copy link
Author

sixers commented Sep 6, 2022

I can foresee this will be very hard to review - I've seen nasty impacts with that type of changes in the past. I'd prefer if you split it into several smaller PRs.

I understand that. This PR is not ready to be fully reviewed and merged, it just highlights all the places that have to be updated to support strictNullChecks. And it also surfaces key decisions that have to be made to fully support strictNullChecks. I believe there is value in looking at all the changes in aggregate, to make sure the API is consistent. It would be great if you could do a high-level review and if we could agree on the general direction, and then I can split the final set of changes into a few smaller PRs.

If it makes things easier, I identified the following main "buckets" for changes.

  • A. Update default values of custom Contexts (e.g. ListContext)
  • B. Update types of async query hooks which return data (e.g. useListContext)
  • C. Update types of async mutation hooks, which expects certain parameters to be passed either to a hook itself, or returned mutation function (e.g. useCreate)
  • D. Bugs, incorrect implicit types, etc. (e.g. in useGetIdentity)

I added a comment for each of the buckets, but I'm also happy to describe each bucket / issue in detail.

If you insist on splitting this work upfront, I can do that as well.

I've seen nasty impacts with that type of changes in the past.

Could you give me an example? It would be helpful for me to get some context and make sure this change doesn't cause issues.

@WiXSL
Copy link
Contributor

WiXSL commented Feb 22, 2023

Related to #7588

@djhi djhi deleted the branch marmelab:next March 24, 2023 10:58
@djhi djhi closed this Mar 24, 2023
@djhi djhi reopened this Mar 24, 2023
@djhi djhi deleted the branch marmelab:next May 25, 2023 08:19
@djhi djhi closed this May 25, 2023
@djhi djhi reopened this May 25, 2023
@djhi
Copy link
Contributor

djhi commented Dec 22, 2023

Thanks for the PR but as @fzaninotto said, we'll work on it in smaller chunks

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.

None yet

5 participants