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

Remove lodash from core #2827

Merged
merged 10 commits into from
May 5, 2021
Merged

Remove lodash from core #2827

merged 10 commits into from
May 5, 2021

Conversation

davwheat
Copy link
Member

@davwheat davwheat commented May 2, 2021

Progresses flarum/issue-archive#179

Changes proposed in this pull request:

Removed lodash-es from our dependencies, and have replaced them with a custom util, a polyfill for a native browser method, and the throttle-debounce library.

Reviewers should focus on:
These replacements don't function identically to the original library's, but should suit our use cases.

Confirmed

  • Frontend changes: tested on a local Flarum installation.

@davwheat davwheat self-assigned this May 2, 2021
@davwheat
Copy link
Member Author

davwheat commented May 2, 2021

-3.4 kB from bundle (-1.65 kB gzipped)

@dsevillamartin
Copy link
Member

Could you not include imports reordering in this PR? That, if at all, should be done outside it.

I'm not in love with copying lodash methods simply to save a few kilobytes, as that means we then are responsible for maintaining those functions, but I guess the ones chosen shouldn't be an issue...

We need a polyfill for iOS 11 and below. I think using a native method with this polyfill is better than having our own function instead, even if the bundle size is ~150B more.
@davwheat
Copy link
Member Author

davwheat commented May 3, 2021

I've switched out the flatten util for the native browser Array.flat(depth) call and a polyfill to support older browsers (read: Safari).

This does make the bundle slightly bigger, but I think it's worth it for the likely better-optimised native browser code and the simplicity of just calling .flat(Infinity). We'll also be able to remove the polyfill in the future to quickly save some more B's!

@davwheat
Copy link
Member Author

davwheat commented May 5, 2021

Just going to see what we gain/lose by using [...this] instead of Array.prototype.slice.call(this). Unsure what Webpack will compile it to.

@davwheat
Copy link
Member Author

davwheat commented May 5, 2021

So it's 366 073 bytes with Array.prototype.slice.call and 366 056 bytes with .... Every little helps! 😂

const realDepth = isNaN(depth as any) ? 1 : depth;

return realDepth
? Array.prototype.reduce.call(
this,
function (acc, cur) {
if (Array.isArray(cur)) {
acc.push.apply(acc, flat.call(cur, realDepth - 1));
(acc as Array<any>).push.apply(acc, flat.call(cur, realDepth - 1));
Copy link
Member

Choose a reason for hiding this comment

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

Can this not be done in the function (acc: ) argument type hinting?

Copy link
Member Author

@davwheat davwheat May 5, 2021

Choose a reason for hiding this comment

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

I would love to if I could figure out what Typescript was continuously complaining about...

I think Array.prototype.reduce.call expects the input to be anything, as it's a generic function, so it just uses any as the types for the accumulator and current value, which plays havoc when we want to use .push.

image

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I can't easily read that xD

Copy link
Member Author

Choose a reason for hiding this comment

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

Y e a h...

I might just type the function args then do as any on the callback to prevent any complaints. That makes it a bit cleaner.

@davwheat davwheat added this to the 1.0 milestone May 5, 2021
@davwheat davwheat merged commit da5db71 into master May 5, 2021
@davwheat davwheat deleted the dw/2345-remove-lodash branch May 5, 2021 23:28
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.

3 participants