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

Optimize lodash. Fixes #6006 #6415

Merged
merged 4 commits into from
Oct 18, 2023

Conversation

Omkar76
Copy link
Contributor

@Omkar76 Omkar76 commented Oct 7, 2023

WHAT

🤖 Generated by Copilot at 73f49f9

This pull request replaces the lodash dependency with lodash-es and imports only the needed functions from the library in various files. This reduces the bundle size and improves the performance of the frontend application. It also refactors some code to use the imported functions directly and consistently.

Proposed Changes

  • Part of remove lodash #6006
  • Replaced lodash with lodash-es.
  • Avoid importing entire package as _. Only import needed utilities

@coronasafe/care-fe-code-reviewers @coronasafe/code-reviewers

HOW

🤖 Generated by Copilot at 73f49f9

  • Replace lodash with lodash-es and import only required functions in package.json and various files to reduce bundle size and support tree-shaking (link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link)
  • Update usage of imported functions from lodash-es to match the import changes in ExternalResultUpload.tsx, Reports/index.tsx, Reports/utils.tsx, ShowInvestigation.tsx, Table.tsx, SampleDetails.tsx, SampleTestCard.tsx, and Notifications.js (link, link, link, link, link, link, link, link, link, link, link, link, link)
  • Remove unused import of lodash from InvestigationTable.tsx (link)

Before optimization

lodash_before

After optimization

lodash after

@Omkar76 Omkar76 requested a review from a team October 7, 2023 07:41
@Omkar76 Omkar76 requested a review from a team as a code owner October 7, 2023 07:41
@netlify
Copy link

netlify bot commented Oct 7, 2023

Deploy Preview for care-egov-staging ready!

Name Link
🔨 Latest commit be98542
🔍 Latest deploy log https://app.netlify.com/sites/care-egov-staging/deploys/6527c6045b95d8000894f223
😎 Deploy Preview https://deploy-preview-6415--care-egov-staging.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@vercel
Copy link

vercel bot commented Oct 7, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
care-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 12, 2023 10:10am

@Omkar76
Copy link
Contributor Author

Omkar76 commented Oct 7, 2023

cc: @sainak

Copy link
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

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

@Omkar76 wasn't the issue about removing and using custom utility functions instead?

This PR is just replacing with another package and updating the imports and doesn't make a noticeable difference in the final bundle size.

We could achieve a much lower bundle size if we built the utility functions within care right?

cc: @sainak

@Omkar76
Copy link
Contributor Author

Omkar76 commented Oct 9, 2023

@Omkar76 wasn't the issue about removing and using custom utility functions instead?

This PR is just replacing with another package and updating the imports and doesn't make a noticeable difference in the final bundle size.

We could achieve a much lower bundle size if we built the utility functions within care right?

cc: @sainak

I believe the size reduction is significant (547 -> 122). We can keep this PR on hold until @sainak has implemented the utilities to replace lodash. If that reduces size significantly more than this we can merge that instead.

@khavinshankar
Copy link
Member

@rithviknishad @sainak can you review this pr once, we'll go ahead with this pr for now and update and keep the original issue open to remove this dependency

Copy link
Member

@sainak sainak left a comment

Choose a reason for hiding this comment

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

Lgtm

@khavinshankar
Copy link
Member

@nihal467 the changes looks good, just give it a general test of above mentioned files

@khavinshankar khavinshankar added the reviewed reviewed by a core member label Oct 10, 2023
@nihal467
Copy link
Member

tested

src/Components/Patient/SampleTestCard.tsx Outdated Show resolved Hide resolved
src/Components/Patient/SampleTestCard.tsx Outdated Show resolved Hide resolved
src/Components/Patient/SampleDetails.tsx Outdated Show resolved Hide resolved
src/Components/ExternalResult/ExternalResultUpload.tsx Outdated Show resolved Hide resolved
@khavinshankar
Copy link
Member

@Omkar76 make the necessary changes suggested by Bodhi

@Omkar76
Copy link
Contributor Author

Omkar76 commented Oct 12, 2023

Hi @bodhish,
Working on your suggestion to use css text-transform instead of startCase and camelCase. From what I'm seeing I think it won't be able to replace those methods . Correct me if wrong

Is a keyword that converts the first letter of each word to uppercase. Other characters remain unchanged (they retain their original case as written in the element's text).
https://developer.mozilla.org/en-US/docs/Web/CSS/text-transform

sampleDetails.status can have value "COMPLETED". We need "Completed". capitalize class won't do that. Similar issue for sampleDetails.doctor_name and maybe in other places.
https://play.tailwindcss.com/wR1YZEkNsO

@rithviknishad
Copy link
Member

rithviknishad commented Oct 12, 2023

You could do:

<span className="capitalize">
  {"COMPLETED".toLowerCase()}
</span> 

// 👉 Completed

@mathew-alex mathew-alex merged commit a92ad24 into coronasafe:develop Oct 18, 2023
31 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants