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.omit #478

Merged
merged 2 commits into from Jun 27, 2017

Conversation

Projects
None yet
3 participants
@balloob
Contributor

balloob commented Jun 27, 2017

When I was analyzing the source map for #474 I noticed that the package lodash.omit takes up 9.29kB!

Looking at the source, we can see that it pulls in a lot of dependent lodash functions.

I've added our own omit function to utils.js, which minifies to 105 bytes, and works with the use cases that we provide to it:

export function omit(obj, omitKeys) {
  const result = {};
  Object.keys(obj).forEach(key => {
    if (omitKeys.indexOf(key) === -1) {
      result[key] = obj[key];
    }
  });
  return result;
}

This saves the user ~9kB.

Paulus Schoutsen
*/
export function omit(obj, omitKeys) {
const result = {};
Object.keys(obj).forEach(key => {

This comment has been minimized.

@mkalish

mkalish Jun 27, 2017

Contributor

Is it worth having a null/undefined check before this line?

@mkalish

mkalish Jun 27, 2017

Contributor

Is it worth having a null/undefined check before this line?

This comment has been minimized.

@balloob

balloob Jun 27, 2017

Contributor

Right now we have the following 2 ways that omit is used:

omit(this.props, ['some', 'keys', 'to', 'omit'])

omit(this.props, Object.keys(propTypes))

That means that both the object and the keys to omit are always passed in.

Right now if you pass in null for either parameter, it will blow up because it will execute Object.keys(null) or null.indexOf(key).

Would you expect obj or omitKeys to be able to be null ?

@balloob

balloob Jun 27, 2017

Contributor

Right now we have the following 2 ways that omit is used:

omit(this.props, ['some', 'keys', 'to', 'omit'])

omit(this.props, Object.keys(propTypes))

That means that both the object and the keys to omit are always passed in.

Right now if you pass in null for either parameter, it will blow up because it will execute Object.keys(null) or null.indexOf(key).

Would you expect obj or omitKeys to be able to be null ?

This comment has been minimized.

@mkalish

mkalish Jun 27, 2017

Contributor

I was thinking of potential of obj to be null, but after a second I agree that it is unnecessary.

@mkalish

mkalish Jun 27, 2017

Contributor

I was thinking of potential of obj to be null, but after a second I agree that it is unnecessary.

Paulus Schoutsen

@eddywashere eddywashere merged commit ba69f71 into reactstrap:master Jun 27, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@eddywashere

This comment has been minimized.

Show comment
Hide comment
@eddywashere

eddywashere Jun 27, 2017

Member

I'd imagine most people already use a ton of lodash so this would be a minor increase in filesize. But for those not using all of it, this will be a big save. 👍

Member

eddywashere commented Jun 27, 2017

I'd imagine most people already use a ton of lodash so this would be a minor increase in filesize. But for those not using all of it, this will be a big save. 👍

@balloob balloob deleted the balloob:omit-omit branch Jun 27, 2017

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