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

Option to not let undefined values override already defined values? #28

Closed
norahmaria opened this issue Jul 12, 2023 · 6 comments
Closed

Comments

@norahmaria
Copy link

Hi! ☺️ I noticed that if you have default values followed by partial versions of the initial object, it returns that everything is possibly undefined.

This might be true as you could specifically set a value to be undefined and that would override it, but if there was an option to avoid this, the type could return the full type rather than a partial version.

This option would make it work more ideally for cases like the example below where you'd want to merge default options with some configurations.

interface Theme {
  button: {
    background: string;
    color: string;
  }
}

const defaultTheme: Theme = {
  button: {
    background: '#a1a1a1',
    color: '#dedede'
  }
}

const themeConfig: DeepPartial<Theme> = {
  button: {
    color: '#fff'
  }
}

/**
 * Everything is possibly undefined.
 * While I'd wish for it to ignore undefined values from themeConfig,
 * and return the full type for Theme, not DeepPartial<Theme> 
 */
const theme = merge(defaultTheme, themeConfig)
@norahmaria norahmaria changed the title Option to not let undefined values override the already defined values? Option to not let undefined values override already defined values? Jul 12, 2023
@voodoocreation
Copy link
Owner

voodoocreation commented Jul 13, 2023

Hmm, the tricky one here is that because you've used the DeepPartial utility type which is saying that everything is possibly undefined, the type returned by ts-deepmerge is a combination of all possible values, which is technically possibly undefined for every property. Another thing to keep in mind is that if you pass in undefined as a value (as opposed to omitting the property from the object), it will override the property with undefined because it's taken as explicitly setting a value of undefined, which is why it has to respect that things may be undefined when the type of the provided object is telling it as such.

However, if you know that the result is definitely going to be Theme, then you can just assign it instead of relying on the inferred return type that ts-deepmerge provides, which is something I've done fairly often, especially when I've used it in the context of deep merging initial Redux state.

You can do it either like this:

const theme: Theme = merge(defaultTheme, themeConfig);

Or like this if you get an error with the above, if the inferred return type fails to align to Theme:

const theme = merge(defaultTheme, themeConfig) as Theme;

These will still allow for sufficient type safety, especially because you're working with objects with assigned types above the merge.

I hope that helps! 🙏

@norahmaria
Copy link
Author

Makes perfect sense @voodoocreation! ☺️ And yeah, the type inferring in this example isn't a necessity as it would be easy to just cast it.

However casting it as Theme in this case would only be safe if there was an option to not let undefined values override existing values, because as of now - the inferred type is correct that there could be undefined values, if one were to pass it explicitly.

In my example it doesn't have any undefined values, but this use case would usually be in a package where the config would likely come from someone using the package, making casting it unsafe.

@voodoocreation
Copy link
Owner

Ahh true. Hmmm... I suppose I could add an option like allowUndefinedOverrides or something, though I'm not too sure how feasible it would be to update the TypeScript magic that infers the types to be able to infer that it's not undefined in that scenario, because I'm not sure if I'd be able to make that operate differently based on the options without defining a separate merge function, which wouldn't be ideal 🤔 would welcome a PR though if you have some ideas? The alternative would be to implement the runtime logic to support disallowing the undefined overrides but leave the return type inferring as-is, which would allow you to assign your types as described above.

Another option could be to handle default values differently in your implementation, such as explicitly defining the options object that uses the provided values with fallbacks to the defaults defined there, though I know it's much more convenient to simply call ts-deepmerge.

I suppose the question is... would you be happy enough if I added the allowUndefinedOverrides option without being able to update the return type inferring if I'm unable to make that work? Just because that way you could assign the types but have the runtime behave the way you want.

@norahmaria
Copy link
Author

I don't think I have the TypeScript skills to infer the correct types for endless params - however I do believe it could be achieved without a separate merge function with discriminating unions based on option.allowUndefinedOverrides.

My solution in the meantime has been a modified version of the merge - which does what I want, but is limited to two parameters.

If you do decide to include the option, maybe the Default type below can spark some ideas. I'm not sure how to translate it to any amount of objects - but if you have ideas how to solve this type, I can create a PR that allows for the types to be inferred depending on the option, without needing two separate merge functions ☺️

import type { DeepPartial } from "./generic.js";

type PickRequired<T> = {
  [L in Exclude<keyof T, OptionalKeys<T>>]: PickRequired<T[L]>;
};

type OptionalKeys<T> = {
  [K in keyof T]-?: {} extends Pick<T, K> ? K : never;
}[keyof T];

type Defaults<T, K> = {
  [L in Exclude<keyof K, OptionalKeys<T>>]: L extends keyof T
    ? T[L] extends object
      ? PickRequired<T[L]>
      : T[L]
    : K[L];
} & T;

interface GenericObject {
  [key: string]: any;
}

export const isObject = (obj: any): obj is object => {
  if (typeof obj === "object" && obj !== null) {
    if (typeof Object.getPrototypeOf === "function") {
      const prototype = Object.getPrototypeOf(obj);
      return prototype === Object.prototype || prototype === null;
    }

    return Object.prototype.toString.call(obj) === "[object Object]";
  }

  return false;
};

export const mergeDefaults = <
  T extends GenericObject,
  K extends DeepPartial<T> = DeepPartial<T>
>(
  defaults: T,
  overrides: K,
  options: {
    mergeArrays: boolean;
    uniqueArrayItems: boolean;
  } = {
    mergeArrays: true,
    uniqueArrayItems: true,
  }
): Defaults<T, K> => {
  /** @Todo Deep clone */
  const copy: GenericObject = { ...defaults };

  Object.keys(overrides).forEach(key => {
    if (["__proto__", "constructor", "prototype"].includes(key)) {
      return;
    }

    if (Array.isArray(copy[key]) && Array.isArray(overrides[key])) {
      if (options.mergeArrays) {
        copy[key] = mergeArrays(
          copy[key],
          overrides[key],
          options.uniqueArrayItems
        );
      } else {
        copy[key] = overrides[key];
      }
    } else if (isObject(copy[key]) && isObject(overrides[key])) {
      copy[key] = mergeDefaults(copy[key], overrides[key], options);
    } else if (overrides[key] !== undefined) {
      copy[key] = overrides[key];
    }
  });

  return copy as Defaults<T, K>;
};

export const mergeArrays = <T, K>(
  initial: T[],
  additions: K[],
  unique: boolean = true
) => {
  const mergedArray = [...initial, ...additions];

  if (unique) {
    return new Set(mergedArray);
  }

  return mergedArray;
};

Personally I'd be happy without the inferred types, but I understand the inferred type is a strength of this package, and as you see, I do have a solution working for me (for two objects) even if you decide not to add this option ☺️

@voodoocreation
Copy link
Owner

voodoocreation commented Jul 17, 2023

Thanks for the snippet - will see what I can come up with. At the very least, I'll add the new option and runtime behaviour - it's just the type inferring that may be tricky. I should have some time either later today or tomorrow to investigate 🙏

@voodoocreation
Copy link
Owner

Hey! Have just released 6.2.0 which has the runtime behaviour via the allowUndefinedOverrides option. I couldn't quite get the TS inferring quite right though, but hopefully this will suffice for now 🙏

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

No branches or pull requests

2 participants