Exists-but-undefined overwrites Graphile Config option on merge

[jcgsville]

Summary

Graphile Config's mergePreset() overwrites options when an option in sourcePreset exists but is undefined. I think it would make more sense if an option that exists but is undefined did not override the option in the targetPreset.

Steps to reproduce

import { resolvePreset } from 'graphile-config'

console.log(
    JSON.stringify(
        resolvePreset({
            extends: [
                {
                    example: {
                        port: 4321,
                    },
                },
            ],
            example: {
                port: undefined,
            },
        }),
        null,
        2,
    ),
)

console.log(
    JSON.stringify(
        resolvePreset({
            extends: [
                {
                    example: {
                        port: 4321,
                    },
                },
            ],
            example: {},
        }),
        null,
        2,
    ),
)

Expected v Actual

These two console logs print out the following. I wish they resolved to the same preset, and thus printed out the same thing.

{
  "plugins": [],
  "disablePlugins": [],
  "lib": {},
  "example": {}
}
{
  "plugins": [],
  "disablePlugins": [],
  "lib": {},
  "example": {
    "port": 4321
  }
}

Motivating Example

I'm working on my graphile-config-example project for library authors. I think this change would make it simpler for library authors to have presets constructed from multiple different places like env vars, CLI args, or a provided config file and merge them. Here's the code that I wrote

import yargs from 'yargs'
import { hideBin } from 'yargs/helpers'
import { loadConfig } from 'graphile-config/load'
import { resolvePreset } from 'graphile-config'

import { run } from './run.js'

const argv = yargs(hideBin(process.argv))
    .option('config', {
        alias: 'C',
        description: 'The path to the config file',
        normalize: true,
    })
    .string('config')
    .option('port', {
        alias: 'P',
        description: 'The port on which the server will listen',
        normalize: true,
        type: 'number',
    })
    .number('port')
    .strict(true)
    .parseSync()

const main = async (): Promise<void> => {
    const userPreset = await loadConfig(argv.config)
    const argvPreset = extendUserPresetWithArgv(userPreset)
    const resolvedPreset = resolvePreset(argvPreset)

    await run(resolvedPreset)
}

const extendUserPresetWithArgv = (
    userPreset: GraphileConfig.Preset | null,
): GraphileConfig.Preset => ({
    extends: userPreset ? [userPreset] : [],
    example: {
        port: argv.port,
    },
})

main().catch((error) => {
    console.error(error)
    process.exit(1)
})

My extendUserPresetWithArgv is overwriting the userPreset. I could modify presetFromArgv to avoid this, but it would be nice for library authors to not have to worry about this.

// This works as I want it to
const extendUserPresetWithArgv = (
    userPreset: GraphileConfig.Preset | null,
): GraphileConfig.Preset => {
    const options: GraphileConfig.ExampleOptions = {}

    if (argv.port !== undefined) {
        options.port = argv.port
    }

    return {
        extends: userPreset ? [userPreset] : [],
        example: options,
    }
}

// This would also work
const extendUserPresetWithArgv = (
    userPreset: GraphileConfig.Preset | null,
): GraphileConfig.Preset => ({
    extends: userPreset ? [userPreset] : [],
    example: {
        port: argv.port ?? userPreset?.example?.port,
    },
})

// The `??=` operator also sets the value to undefined, so this also doesn't work:
const extendUserPresetWithArgv = (
    userPreset: GraphileConfig.Preset | null,
): GraphileConfig.Preset => {
    const options: GraphileConfig.ExampleOptions = {}
    options.port ??= argv.port

    return {
        extends: userPreset ? [userPreset] : [],
        example: options,
    }
}

Possible Solution

If an option exists but undefined (i.e. { port: undefined }, then it should not overwrite the corresponding property in the targetPreset.

I propose that exists-but-null be the way that users indicate "I'm trying to overwrite the preset I'm extending but explicitly disabling this option".

I could see the argument for the other side. Maybe the existence of the property driving overwrite behavior is a bit clearer / simpler behavior, but I do think it makes dev ergonomics a bit worse.

I'm willing to make the change to graphile config if we agree that this is a good change to make 🙂 It should be a simple change somewhere around here:

crystal/utils/graphile-config/src/resolvePresets.ts

Line 384 in dd081e8

(targetPreset as any)[scope] = Object.assign(

It has the potential to be a bit dangerous for existing graphile libraries that use graphile config. Wouldn't surprise me if some behavior would subtly change.

Even if we decide not to make a change, I'd like to make an addition to Graphile Config docs to call this out explicitly in the pseudocode for extend preset

[benjie]

If this were made the case, we couldn’t “unset” something that was set, e.g. so that a default could be used. This may be particularly important for organisations that create a base shared preset, but now someone wants to override one of the options to use the library defaults (without having to keep them in sync). It’s relatively easy (though not particularly ergonomic) to not set keys that are undefined by using spread ternaries; or a helper function could be used to strip undefineds from an object.

I’m unconvinced that this is a sufficiently desirable feature purely for ergonomics to justify preventing a capability that cannot otherwise be expressed.

[jcgsville]

Couldn't someone unset a feature by setting it to null? I guess, now that I'm thinking deeply about it, that would require the library author to explicitly include null as an allowable type in the options they define.

Maybe even that requirement outweighs the minor ergonomic benefit I suggested

[jcgsville]

Continuing to reflect on this. To some degree, I actually like requiring library authors to make null an option if they want people to be able to "unset" a property.

Take for example, a module that accepts a port option that is ultimately required for the module to function. There's not really a use case for unsetting port. You either want the existing value, or you want to change it to a new value. In this case, the author might not allow null, and thus not allow unsetting:

declare global {
    namespace GraphileConfig {
        interface ExampleOptions {
            port?: number | undefined
        }

        interface Preset {
            example?: ExampleOptions
        }
    }
}

Then, for other options where it would be useful for users to unset it, you can use ActualType | null | undefined

[jcgsville]

Also, didn't mean to label as a bug. Looks like I can't remove label though

[benjie]

Setting null and unsetting are two different things in my opinion, for example maskErrors: null would mean “don’t mask errors” whereas maskErrors: undefined would mean “nevermind about the override from the V4 preset to maskErrors; please just use your default”. When the library uses options from the config it would then do something like:

const {
  port = parseInt(process.env.PORT, 10) || 5678,
  maskError = defaultMaskError
} = preset.scope || {}

[benjie]

All config options are optional, thus all must have some kind of default in the library; undefined lets you reference the default without having to look up what it is.

[jcgsville]

I see. In my opinion, I think that's a bit on the confusing side. I'm not sure there's a strong use case for "unset the value from the preset but set it back to the library default". I think most people are going to think of extending a preset as using a default. Wheras this behavior makes library users have to consider multiple layers of defaults, and the ability to remove the layer above you in the tree is a bit weird.

But you've certainly thought longer about what postgraphile/grafast/etc users need than I have 🙂, and I guess JS forces our hand here and there's no choice here that does not involve some documentation of expected behavior and reliance on library authors to conform to patterns :)

(PS I really like that Rust got rid of null 😅. Explicitness ftw! JS is the polar opposite with 3x versions of it haha)

So if I understand correctly, we want to prioritize the experience for preset creators vs library authors when those are in tension - I think this is the correct call

Thus, if I'm a library user creating a preset:

• Not setting an option means "use whatever value is in the preset I'm extending"
• Setting an option to undefined means "if the preset I'm extending set a value for this option, unset it and go back to whatever the library provides as default"
• Assuming the TS types for the options allow null, null has the same override behavior as setting the option to a non-null non-undefined values

The downside of this is that library authors need to be a bit more careful when they're creating & extending presets programmatically from things like CLI arguments or default values.

I'll see if there's a good way to incorporate a bit more clarity on this into the docs

[benjie]

Yes that sounds like an accurate summary. Essentially it follows the behaviour of Object.assign(...) (i.e. if a key is present, overwrite the value) - this is actually mentioned in the preset resolving algorithm - and enables the behaviour of default values during destructuring (const { foo: { a = defaultA } = {} } = preset) which would not work if the value were null. I feel that it's embracing the way that these built in features of JS works rather than layering additional behaviours on top.