-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[@mantine/dates] fix: date picker input memory leak when string is empty #7780
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
Conversation
Managed to find a better fix, which is to check where exactly the error is happening and if the value is invalid it then defaults to todays date. This then renders the picker and no longer leaks memory - I will say, this might be more of an issue on dayjs than on the dates package but Im not sure This at least is more of a solid fix than type coercion and should also fall back to a better default state. Should realistically never happen unless the initial value being set is gibberish. Edit: as pointed out here by dayjs docs, null is invalid so we should handle this in some way to have some form of default value (like "today") when it is an invalid value like |
Simple solution for this is do it in utils/to-dates-string, I simply applied in pnpm patch seems working well function toDateString(value) {
return value == null || value === '' ? value: dayjs(value).format("YYYY-MM-DD");
}
function toDateTimeString(value) {
return value == null || value === '' ? value: dayjs(value).format("YYYY-MM-DD HH:mm:ss");
} |
Good one, I'll apply this tomorrow morning to this pr |
Applied to util. Tested locally too and it works like a charm. Has similar output to initial default except it has a more intuitive feel to it when a date may be invalid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses a memory leak issue in the date picker input by modifying the handling of empty strings.
- Updated the conditional logic in toDateString and toDateTimeString functions.
- Prevents dayjs from processing empty string values that can lead to unwanted behavior.
return value == null || value === '' ? value : dayjs(value).format('YYYY-MM-DD'); | ||
} | ||
|
||
export function toDateTimeString( | ||
value: string | number | Date | Dayjs | undefined | null | ||
): DateStringValue | undefined | null { | ||
return value == null ? value : dayjs(value).format('YYYY-MM-DD HH:mm:ss'); | ||
return value == null || value === '' ? value : dayjs(value).format('YYYY-MM-DD HH:mm:ss'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the PR description, an empty string should be interpreted as null. Consider returning null instead of an empty string to better align with the docs and prevent potential inconsistencies.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR description is out of date, follow up comments are most relevant.
return value == null || value === '' ? value : dayjs(value).format('YYYY-MM-DD'); | ||
} | ||
|
||
export function toDateTimeString( | ||
value: string | number | Date | Dayjs | undefined | null | ||
): DateStringValue | undefined | null { | ||
return value == null ? value : dayjs(value).format('YYYY-MM-DD HH:mm:ss'); | ||
return value == null || value === '' ? value : dayjs(value).format('YYYY-MM-DD HH:mm:ss'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the PR description, an empty string should be interpreted as null. Consider returning null instead of an empty string to ensure consistent behavior based on the docs.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Description is out of date, follow up comments provide most context.
Thanks! |
Adresses #7779
Seems to be some problem going on with the date picker inputs not allowing empty strings properly and causing some kind of memory leak. For now I propose this as an easy fix, however I will take time later to look into a more sufficient fix.
This solution at least for now will stop any memory leak from happening and cause anything empty string to be interpreted asnull
-> I assume this is the behaviour we want anyway due to the docs usingnull
as the default value anyway.More sufficient fix; in utils check for empty string.