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

Use lodash method imports & replace some with native functions #126

Closed
wants to merge 3 commits into from

Conversation

realityking
Copy link
Contributor

I know there's been some recent bad experience with lodash refactors but I hope this more conservative approach here is safer.

The PR does 2 things:

  • It uses the lodash per method imports. This loads less code but more importantly it makes it easier to see what parts of lodash are actually used by table
  • It uses the native functions String.prototype.trimEnd and Object.values. Both are available now that table requires Node.js 10

Some other things might be possible but I think they're better kept for follow-up PRs.

Note I did not use the per-method packages as they're deprecated and actually hurt deduplication. They also load more code than this approach.

@coveralls
Copy link

coveralls commented Nov 6, 2020

Pull Request Test Coverage Report for Build 216

  • 16 of 19 (84.21%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 72.948%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/truncateTableData.js 0 1 0.0%
src/createStream.js 1 3 33.33%
Totals Coverage Status
Change from base Build 215: 0.0%
Covered Lines: 172
Relevant Lines: 236

💛 - Coveralls

@gajus
Copy link
Owner

gajus commented Nov 6, 2020

table/.babelrc

Line 10 in 623350b

"babel-plugin-lodash",

@gajus gajus closed this Nov 6, 2020
@realityking realityking deleted the lodash branch November 19, 2020 17:10
@realityking
Copy link
Contributor Author

My bad, I didn't see that. I've opened #127 for the other two commits in this branch as they still make sense.

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.

3 participants