Skip to content

Use lodash individual modules instead of requiring the entire package#102

Merged
kyrylo merged 3 commits intoairbrake:masterfrom
davej:feature/use-lodash-modules
Jun 21, 2016
Merged

Use lodash individual modules instead of requiring the entire package#102
kyrylo merged 3 commits intoairbrake:masterfrom
davej:feature/use-lodash-modules

Conversation

@davej
Copy link
Copy Markdown
Contributor

@davej davej commented Jun 21, 2016

I'm using node-airbrake with electron so size is an issue for me. lodash adds 1.3MB to my app so I'm hoping you can use the individual packages instead.

lib/airbrake.js Outdated
}

_.assign(Airbrake.prototype, EventEmitter.prototype);
assign(Airbrake.prototype, EventEmitter.prototype);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can't we use merge here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I believe assign is more performant as it doesn't deep merge. I don't really know though, I didn't want to change any of the logic in this PR. You could also use Object.assign(), but that is only available in Node v4 and later.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think performance really matters here. May I ask you to use only merge, given that we don't use assign anywhere else?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. I can make the change but I can't say for sure that it wouldn't break something.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Change pushed

@kyrylo
Copy link
Copy Markdown
Contributor

kyrylo commented Jun 21, 2016

Looks good, thanks! Could you squash into 1 commit?

@davej
Copy link
Copy Markdown
Contributor Author

davej commented Jun 21, 2016

I can if you want. Do you know that you can do a one-click squash and merge from github? Just click the arrow next to the merge button below. It's a bit nicer because it maintains the history in the PR. :-)

@kyrylo kyrylo merged commit 0fb12d8 into airbrake:master Jun 21, 2016
@kyrylo
Copy link
Copy Markdown
Contributor

kyrylo commented Jun 24, 2016

Just released v1.0.3 with your change. Happy Airbraking!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants