fix: remove date-fns as external package #5128
Conversation
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network for review. Expert reviewers are now being matched to your request based on the code's requirements. Stay tuned!
What to expect from this code review:
- Comments posted to any areas of potential concern or improvement.
- Detailed feedback or actions needed to resolve issues that are found.
- Turnaround times vary, but we aim to be swift.
@johannrp27 you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
We shouldn't necessarily remove this just because it is causing issues.
Could you please specify the before/after bundle size in the PR description from making these changes? It is possible that it negatively impacts bundle size to remove those entries.
By the way, these changes don't actually remove date-fns as an external dependency. It just removes the tree-shakable approach where we specify every date-fns module.
...Object.keys(pkg.dependencies || {}) still includes date-fns. So it is worth noting that every package inside of dependencies and peerDependencies is marked as external, and that is intentional. Specifying each module of date-fns allowed us to improve bundle size for date-fns modules that were not being used by ensuring they don't get included.
Reviewed with ❤️ by PullRequest
There was a problem hiding this comment.
We shouldn't necessarily remove this just because it is causing issues.
Could you please specify the before/after bundle size in the PR description from making these changes? It is possible that it negatively impacts bundle size to remove those entries.
By the way, these changes don't actually remove
date-fnsas an external dependency. It just removes the tree-shakable approach where we specify everydate-fnsmodule.
...Object.keys(pkg.dependencies || {})still includesdate-fns. So it is worth noting that every package inside ofdependenciesandpeerDependenciesis marked as external, and that is intentional. Specifying each module ofdate-fnsallowed us to improve bundle size fordate-fnsmodules that were not being used by ensuring they don't get included.
Having said that, if a quick fix is desired to get the website back online, this should work. But we should ensure that the resulting package is not published until a root cause and proper fix have been determined.
|
Yes, you're right. This fix increases the bundle size a lot. I'll look into it and tell you. Thanks for the observation |
name: fix: remove date-fns as external package
about: Update of date-fns library from 3.6 to 4.1 was running into a issue with current rollup configuration
title: Remove date-fns as external package
labels: ""
assignees: "martijnrusschen"
Description
Linked issue: #5108
Problem
Get back online page with latest version of date-fns
Changes
Rollup issue remove
Contribution checklist