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] Make types more strict in ra-ui-materialui, part III #9795

Merged
merged 8 commits into from
Apr 29, 2024

Conversation

fzaninotto
Copy link
Member

@fzaninotto fzaninotto commented Apr 28, 2024

Reduces strictNullChecks compilation errors in ra-ui-materialui/src/inputs directory from 109 to 50

@@ -1,7 +1,8 @@
import { useEffect } from 'react';
import { useStore } from '../store';
import { debounce } from 'lodash';
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is unrelated to the PR, but I noticed that the bundles for the demos were embarking the entire lodash, so I figured I'd fix this regression.

@@ -62,7 +62,7 @@ describe('<useList />', () => {
{ id: 2, title: 'world' },
{ id: 1, title: 'hello' },
],
error: undefined,
Copy link
Member Author

Choose a reason for hiding this comment

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

this is to make the output compatible with the ListControllerResult, which is using useQueryResult signature. In it, the empty error is null, not undefined.

@@ -485,7 +485,7 @@ export interface ListControllerLoadingResult<RecordType extends RaRecord = any>
error: null;
isPending: true;
}
export interface ListControllerLoadingErrorResult<
Copy link
Member Author

Choose a reason for hiding this comment

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

this name didn't make sense

@@ -1,11 +1,11 @@
id,reference,date,customer_id,basket.product_id,basket.quantity,total_ex_taxes,delivery_fees,tax_rate,taxes,total,status,returned
408,5TFGTA,2021-04-23T02:07:38.273Z,17,87,1,108.28,5.38,0.2,22.73,136.39,ordered,false
Copy link
Member Author

Choose a reason for hiding this comment

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

The customer_ids weren't pointing to actual records in the customers csv, which caused warnings on import

@@ -46,8 +44,8 @@ export const SimpleFormIterator = (inProps: SimpleFormIteratorProps) => {
});
const {
addButton = <DefaultAddItemButton />,
removeButton = <DefaultRemoveItemButton />,
Copy link
Member Author

Choose a reason for hiding this comment

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

moved default values to SimpleFormIteratorItem, where they make more sense

@@ -185,7 +186,7 @@ export const CheckboxGroupInput: FunctionComponent<CheckboxGroupInputProps> = pr
[allChoices, formOnChange, formOnBlur, optionValue, value]
);

if (isPending && (!allChoices || allChoices.length === 0)) {
if (isPending) {
Copy link
Member Author

Choose a reason for hiding this comment

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

already tested L136

@@ -72,12 +72,7 @@ export const useReferenceArrayFieldController = <
const notify = useNotify();
const value = get(record, source);
const { meta, ...otherQueryOptions } = queryOptions;

const ids = useMemo(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

chore: 'useMemo' is imported line 2 but no more used‏, we may could remove it

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, fixed

Comment on lines +129 to +130
formGroups &&
formGroupName != null &&
Copy link
Contributor

Choose a reason for hiding this comment

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

thought (non-blocking): ‏ Not sure, but may we add finalSource in the condition ?

Suggested change
formGroups &&
formGroupName != null &&
formGroups &&
finalSource &&
formGroupName != null &&

Copy link
Member Author

Choose a reason for hiding this comment

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

finalSource is always defined because source is required (see ArrayInputProps and InputProps).

Comment on lines +135 to +136
formGroups &&
formGroupName != null &&
Copy link
Contributor

Choose a reason for hiding this comment

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

thought (non-blocking): ‏ Same here

Suggested change
formGroups &&
formGroupName != null &&
formGroups &&
finalSource &&
formGroupName != null &&

document.elementFromPoint(x.current, y.current) === null
? selectedItem
: document.elementFromPoint(x.current, y.current).closest('li');
: // @ts-ignore TypeScript is drunk
Copy link
Contributor

@adguernier adguernier Apr 29, 2024

Choose a reason for hiding this comment

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

Indeed 🥴

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nope, extract the result of document.elementFromPoint in a variable

@@ -97,7 +98,7 @@ export const useSupportCreateSuggestion = (

if (finalValue?.id === createValue || finalValue === createValue) {
if (!isValidElement(create)) {
const newSuggestion = await onCreate(filter);
const newSuggestion = await onCreate!(filter);
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm not mistaken, onCreate could be null because is not mandatory in SupportCreateSuggestionOptions interface.

@djhi djhi merged commit 4969243 into next Apr 29, 2024
12 checks passed
@djhi djhi deleted the fix-strict-null-checks-mui-input branch April 29, 2024 15:01
@fzaninotto fzaninotto added this to the 5.0.0 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants