-
-
Notifications
You must be signed in to change notification settings - Fork 3
Remove lodash-es dependency #93
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
🦋 Changeset detectedLatest commit: 68d26ed The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@layerstack/svelte-actions
@layerstack/svelte-state
@layerstack/svelte-stores
@layerstack/svelte-table
@layerstack/tailwind
@layerstack/utils
commit: |
built with Refined Cloudflare Pages Action⚡ Cloudflare Pages Deployment
|
|
Hey @es3n1n, thanks so much! 🫶. This will provide a great reduction in bundle size. To answer your questions:
While I try to target a broad range of browsers when possible, I typically don't stress over browsers more than 2-3 years old unless there is a strong use case to do so. Considering Safari 9 is almost 10 years old, I'm not overly concerned maintaining compatibility (and we've likely broken it elsewhere).
This sounds robust enough for me, unless we had any usage otherwise (off the stop of my head I can't think of anything else).
Sorry, but yes we'll need to target the Regarding LayerChart specifically, a few points to note:
With that said, when we go to update LayerChart with these changes, we'll want to do this on at least Sorry that we've kicked up a fair bit of dust during this process, but we're making great progress (I hope to finish / merge Thanks again for all the help! |
|
oh, btw the reduced bundle sizes will be even more impactful as we now provide StackBlitz links for all 700+ examples, and are also working on a new integrated playground. It's a bit slow at the moment due to the package installs and lodash/deps was very much top of mind to improve this, so this is great timing :) |
1d9e4d1 to
09b7888
Compare
|
Alright, I've rebased to next and left only the stuff we're actually using. Once this PR is merged, I'll probably start looking at the layerchart. I'll make sure to base my changes on the |
techniq
left a comment
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.
Looks good! Thanks for the comprehensive tests.
One last request, can you run pnpm changeset (patch) to update the CHANGELOG and trigger the CI workflows to deploy a npm release.
Thanks!
|
We should be good to go now! |
|
@es3n1n all packages published.
Thanks again, and looking forward to the LayerChart improvements! |
Hello,
I'm slowly starting to address techniq/layerchart#616. Figured that getting rid of lodash might be a good start.
Essentially, I reimplemented all the required functions that are used in the layerstack itself, plus
memoize, which is used in layerchart. I tried to add more tests to the new pieces of code to ensure the same behavior compared to lodash, but please note that most of them were created using an LLM. I also tried to reference the originally linked just implementations where possible.Here are some implementaiton notes/questions:
For the
isFunctionimplementation there are some additional pieces of code we might want to add to support safari 9, but given that it was released in 2015, perhaps we can just ignore that?For the
isEqualimplementation, do we want to support anything other than dates, maps, sets, arrays, objects?closes #78
Also should this target the next branch instead? Kind of realized there's an active next branch only after i opened a pr, sorry