fix(gatsby-plugin-sharp): Handle diff duotone settings

[LekoArts]

Description

The createArgsDigest(options) receives an options with the following input:

options {
  key: 'value',
  duotone: [Object: null prototype] { highlight: '#F1D283', shadow: '#000000' }
}

The node-object-hash doesn't recognize the [Object: null prototype] as an object an gives it the unknown type. So in the hashing then the contents of duotone don't make a difference.

We probably didn't catch this yet as while transformOptions also has the same null prototype (Googling pointed me towards GraphQL), we spread it: https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-plugin-sharp/src/image-data.ts#L220

Since duotone is the only key that is an object, the one-off destructuring for duotone is fine IMO

Related Issues

Fixes #35054

[wardpeet]

Should we also try to fix it in the upstream dependency because there might be other cases we're not aware of

[LekoArts]

Should we also try to fix it in the upstream dependency because there might be other cases we're not aware of

A "fix" for it would be JSON.parse(JSON.stringify(stuff)) and I'm not sure if they'd accept that. I've looked through our options and duotone is the only one that is an object, the rest is string, number etc so no null protoype

[imjoshin]

Should we also try to fix it in the upstream dependency because there might be other cases we're not aware of

A "fix" for it would be JSON.parse(JSON.stringify(stuff)) and I'm not sure if they'd accept that

Here's a working change at https://github.com/SkeLLLa/node-object-hash/blob/master/src/objectSorter.ts#L208:

var objectName = typeof obj.toString === 'function' ? obj.toString() : 'unknown';

if (objectName === 'unknown') {
    try {
        var json = JSON.stringify(obj)
        if (json) {
            objectName = json
        }
    } catch {
        // pass
    }
}

I think it may be worth asking so we don't get bit by this again. I could also see it as an opt-in flag on their config.

[pieh]

is this correct ? It seems to me like it would "flatten" duotone:

// some example
const args = { foo: `bar`, duotone: { wat: true }, a: 5 }
const { pathPrefix, duotone, ...rest } = args
const TEST = Object.assign(rest, duotone)
// TEST = {foo: 'bar', a: 5, wat: true}

[LekoArts]

options is only used for the digest, args is used for options return so should be fine

[pieh]

oh, right - then my suggestions would be to rename options to something else just to indicate it's not really "options" anymore as shape is different (just to avoid confusion as I had above if someone read it some time later). Ideally with some comment with context

[LekoArts]

@pieh Done

[pieh]

I wonder if instead we should adjust hasher to handle null prototype? So that we don't have to handle cases like that in the future in other plugins. To me that would be more reliable fix (tho might be much more involved?)