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

chore: replaced lodash function packages #303 #309

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

staxie
Copy link

@staxie staxie commented Feb 27, 2023

No description provided.

@danpaz
Copy link
Owner

danpaz commented Mar 10, 2023

Thanks for the PR but I don't think we should take this approach, this essentially just reverts the work that was done in #291 which reduced the bundle size significantly.

@mrslwiseman
Copy link

Would love to get this issue fixed, as we use this lib at work and the vulnerabilities found in our security audit pipeline are bugging our team ( we use snyk.io for vuln scanning ).

Moving forward the options I can think of ( in no particular order )

  • find a well considered implementation out there and copy/paste
  • find an alternative lib ( again, maintenance issue )
  • use lodash as per this PR ( not maintained though )

I'm happy to contribute or if @staxie wants to do it I'm happy to contribute somehow

@mrslwiseman
Copy link

Another point regarding bundle size - I'd assume most people are using this lib with nodejs, does bundle size really matter in that case? For frontend, surely most devs would have tree shaking enabled? Perhaps I'm mistaken...

@johannes-scharlach
Copy link
Collaborator

johannes-scharlach commented Mar 18, 2023

@mrslwiseman I think it should be relatively straightforward to remove the usage of lodash.set entirely.

simple cases like set(clonedBody, 'aggs', aggregations) can be replaced with clonedBody.aggs = aggregations and complex cases such as set(queryBody, 'query.bool.must', queries) should be simple enough with

queryBody.query = queryBody.query || {}
queryBody.query.bool = queryBody.query.bool || {}
queryBody.query.bool.must = queries

which does exactly the same as lodash.set in our cases.

It only appears in src/index.js, so this change should be easy enough to do.

Alternatively, write a 3-line custom set function. This is what chatGPT spit out when I asked to implement lodash.set:

function set(obj, path, value) {
  const keys = Array.isArray(path) ? path : path.split('.'); // convert path to array if it's a string
  const lastKeyIndex = keys.length - 1;

  for (let i = 0; i < lastKeyIndex; i++) {
    const key = keys[i];
    if (!(key in obj)) {
      obj[key] = {};
    }
    obj = obj[key];
  }

  obj[keys[lastKeyIndex]] = value;
}

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.

4 participants