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

Deep merge meta objects #1885

Closed

Conversation

okaybroda
Copy link

@okaybroda okaybroda commented Feb 7, 2021

Change meta merge from Object.assign to lodash merge to allow deep merge of meta objects

Closes #1884

@FoxxMD
Copy link

FoxxMD commented Jun 2, 2021

I'd like to request a second, optional param added to child() so a customizer function can be applied. I want merge deep functionality but also want arrays to be merged...named values (including arrays) are still overwritten by merge but can be addressed by using mergeWith and a customizer function:

  child(defaultRequestMetadata, customizer = undefined) {
    const logger = this;
    return Object.create(logger, {
      write: {
        value: function (info) {
          const infoClone = mergeWith(
            {},
            defaultRequestMetadata,
            info,
            customizer
          );

          // Object.assign doesn't copy inherited Error
          // properties so we have to do that explicitly
          //
          // Remark (indexzero): we should remove this
          // since the errors format will handle this case.
          //
          if (info instanceof Error) {
            infoClone.stack = info.stack;
            infoClone.message = info.message;
          }

          logger.write(infoClone);
        }
      }
    });
  }

@vincentjames501
Copy link

+1

@JaneJeon
Copy link

This sounds like a no-brainer. Does any maintainer have any bandwidth for this?

@FoxxMD
Copy link

FoxxMD commented Oct 25, 2021

@JaneJeon I ended up forking and implementing my example if you want to use it https://github.com/FoxxMD/winston/tree/feature/deep-merge-meta-object

@maverick1872
Copy link
Member

I'd like to request a second, optional param added to child() so a customizer function can be applied. I want merge deep functionality but also want arrays to be merged...named values (including arrays) are still overwritten by merge but can be addressed by using mergeWith and a customizer function:

  child(defaultRequestMetadata, customizer = undefined) {
    const logger = this;
    return Object.create(logger, {
      write: {
        value: function (info) {
          const infoClone = mergeWith(
            {},
            defaultRequestMetadata,
            info,
            customizer
          );

          // Object.assign doesn't copy inherited Error
          // properties so we have to do that explicitly
          //
          // Remark (indexzero): we should remove this
          // since the errors format will handle this case.
          //
          if (info instanceof Error) {
            infoClone.stack = info.stack;
            infoClone.message = info.message;
          }

          logger.write(infoClone);
        }
      }
    });
  }

@FoxxMD Would you be willing to create a Feature Request for this? It's related but we shouldn't be introducing new functionality in a Bugfix PR.

@FoxxMD
Copy link

FoxxMD commented Jan 14, 2022

@maverick1872 absolutely. Should I wait until #1989 is merged/finalized before opening a PR?

@maverick1872
Copy link
Member

Since they're unrelated no need to wait. Although I will ask if you can give me a day or two prior to creating it. I'm working on introducing some changes to our issue/feature templates to hopefully help with some housekeeping moving forward.

@maverick1872
Copy link
Member

@FoxxMD Issue templates updated - feel free to create feature request whenever.

@maverick1872
Copy link
Member

Tested your implementation against the new test cases I've developed in #1989. 10 of them are failing so I think it best to close this PR for an alternative implementation. Additionally I'm not a huge fan of introducing new dependencies unless absolutely required.

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.

Deep merge meta objects
5 participants