perf: remove momentjs#16615
Conversation
|
Linear: CAT-1535 |
|
✅ Meticulous spotted 0 visual differences across 1523 screens tested: view results. Meticulous evaluated ~9 hours of user flows against your PR. Expected differences? Click here. Last updated for commit 4477b54. This comment will update as new commits are pushed. |
Bundle ReportChanges will decrease total bundle size by 234.63kB (-1.02%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: datahub-react-web-esmAssets Changed:
Files in
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
ani-malgari
left a comment
There was a problem hiding this comment.
Looks like we're also importing dayjs directly in some places and when we call it's functions, it silently depends on it's plugins having to be imported in @utils/dayjs and might throw an error if dayjs hasn't been mutated already. The patterns are also inconsistent in some places.
The ideal case is to use @utils/dayjs in all imports, and enforce a ESLint rule to import from utils. It can increase the scope of the PR, and it's okay if we want to do a separate immediate follow-up PR.
I agree! Thanks for flagging this. |
ani-malgari
left a comment
There was a problem hiding this comment.
the PR looks great except i have two comments:
- In
datahub-web-react/src/app/search/filters/utils.tsx, we've replaced a constant with a prop and not passing this prop from the callers. i'm afraid this will break or return an undesired color, we need to fix this before we merge. - also curious why we still have moment in lock file. is it a caching issue?
| moment ">= 2.9.0" | ||
|
|
||
| "moment@>= 2.9.0", moment@^2.24.0, moment@^2.29.2, moment@^2.29.4: | ||
| moment@^2.24.0, moment@^2.29.2: |
There was a problem hiding this comment.
do you know what this is for?
There was a problem hiding this comment.
moment is still pulled in by Antd. The 360kB reduction in bundle is from dropping moment-timezone.
| filterEntity: Entity | null, | ||
| entityRegistry: EntityRegistry, | ||
| size?: number, | ||
| iconColor?: string, |
There was a problem hiding this comment.
why are we creating a new prop for iconColor, which was earlier a constant?
There was a problem hiding this comment.
and we haven't updated iconColor to the caller of this function. this i'm afraid will break or return an undesired default color at the entityRegistry.getIcon call.
There was a problem hiding this comment.
You're right. The proper fix would be to useTheme and get color from there in all locations but that's outside of the scope of this PR. Since the eslint rule is now disabled, I will revert to original code.
|
|
||
| export const TimezoneSelect = ({ value, onChange }: Props) => { | ||
| const timezones = moment.tz.names(); | ||
| const timezones = (Intl as any).supportedValuesOf('timeZone') as string[]; |
There was a problem hiding this comment.
why do we need to do a cast here?
There was a problem hiding this comment.
We use Typescript 4.8.4 and according to microsoft/TypeScript#49231 supportedValuesOf landed somewhere in 5.x. I suppose we could extend Intl namespace manually, as suggested in the issue but I think any might be ok here.
|
|
||
| export const TimezoneSelect = ({ value, onChange }: Props) => { | ||
| const timezones = moment.tz.names(); | ||
| const timezones = (Intl as any).supportedValuesOf('timeZone') as string[]; |
There was a problem hiding this comment.
Does dayjs not provide a list of timezones?
There was a problem hiding this comment.
I don't think it does: https://day.js.org/docs/en/timezone/timezone
| const hours = Math.floor(duration.asHours()); | ||
| const minutes = duration.minutes(); | ||
| const seconds = duration.seconds(); | ||
| const dur = dayjs.duration(durationMs); |
There was a problem hiding this comment.
nit: I don't think there's a strong reason to shorten the name here
Replace moment.js with day.js.
Scope: 30 files use moment, 11 use moment-timezone. API is nearly identical.
Net saving: ~318 KB gz. Risk: Low (API compatible, dayjs already in bundle, timezone plugin already used in 3 places).