Should we use `hasOwn` instead of `hasOwnProperty`?

[emab]

I get a few warnings from ESLint and other linting tools when generating mock data due to the usage of hasOwnProperty.

It seems the recommended way to check for a property on an object is to use Object.hasOwn which seems to have widespread browser adoption nowadays.

The generated output would behave in the same way, but would look like this instead:

export const aUser = (overrides?: Partial<User>): User => {
   status: overrides && Object.hasOwn(overrides, 'status') ? overrides.status! : Api.Status.Online,
}

Is this something you'd be interested in me implementing? Is it worthwhile?

Browser compatibility

_Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/hasOwn#browser_compatibility_

[ardeois]

That's a good idea but hasOwn seems quite new as it's supported on NodeJS 16 only This would mean we would drop support for Node JS 14 and 15

Maybe we could use 'status' in overrides instead ?

[emab]

Ah didn't realise it needed Node 16 support! I was thinking about suggesting in actually.

The only problem is that it will return true for inherited properties as well as non-inherited - for example if someone had a field called hasOwnProperty then when we call 'hasOwnProperty' in overrides it will return true even if they didn't override it. This will be true for any of the other object prototypes.

Potentially not so bad but definitely something to keep in mind.

Another way which is slightly less neat could be to do something like:

overrides && (overrides?.property) as TypeName["property"] ? overrides.property! : "generated";

Did this on mobile so unsure if this would work - could also cast overrides using some type too potentially.

[ardeois]

Ah interesting, I didn't know that in would be true for object prototypes...

For overrides && (overrides?.property) it won't work because we won't be able to override a value to undefined. For example, an override object like this { foo: undefined} would be ignored and the default value would be used. That's a bug we fixed a while ago.

Another option is to ignore eslint for generated files by using the add plugin ? Here is an example https://the-guild.dev/graphql/codegen/plugins/other/add#examples

Also, seems like ({}).hasOwnProperty.call(obj, key) would be an equivalent of hasOwnProperty https://stackoverflow.com/a/45216040

[emab]

Ah yep great point. Feels like the current method is the safest one for now. Maybe once hasOwn is more widely adopted this might be an easier decision - not worth bumping another major version for this.

Lots of chat about hasOwn in that StackOverflow page but also lots of people pointing out it is was still not properly implemented in Safari as near as a few months ago...

[emab]

We could take a different approach to this altogether actually which I think might work:

type CoolType = {
    action: string;
    url: string;
    count: number | undefined;
    isActive: boolean;
}

const aCoolType = (
    overrides?: Partial<CoolType>,
    _relationshipsToOmit: Set<string> = new Set()
): CoolType => {
    const relationshipsToOmit: Set<string> = new Set(_relationshipsToOmit);
    relationshipsToOmit.add("CoolType");
    return {
        action: overrides && "action" in overrides ? overrides.action! : "Do thing",
        url:  overrides && "url" in overrides ? overrides.url! : "http://example.com",
        count:  overrides && "count" in overrides ? overrides.count! : 10,
        isActive:  overrides && "isActive" in overrides ? overrides.isActive! : true,
    }
}

const aCoolTypeNew = (
    overrides?: Partial<CoolType>,
    _relationshipsToOmit: Set<string> = new Set()
): CoolType => {
    const relationshipsToOmit: Set<string> = new Set(_relationshipsToOmit);
    relationshipsToOmit.add("CoolType");
    return {
        action: "Do thing",
        url: "http://example.com",
        count:  10,
        isActive: true,
        ...overrides,
    }
}

const overrides = [{

}, {
    action: "New cta"
},
{
    url: "New url",
    isActive: false
}, {
    count: undefined,
    action: "Hello world"
}] as Partial<CoolType>[];

for (const override of overrides) {
    console.log(aCoolType(override), aCoolTypeNew(override))
}

Output:

[LOG]: {
  "action": "New cta",
  "url": "http://example.com",
  "count": 10,
  "isActive": true
},  {
  "action": "New cta",
  "url": "http://example.com",
  "count": 10,
  "isActive": true
} 
[LOG]: {
  "action": "Do thing",
  "url": "New url",
  "count": 10,
  "isActive": false
},  {
  "action": "Do thing",
  "url": "New url",
  "count": 10,
  "isActive": false
} 
[LOG]: {
  "action": "Hello world",
  "url": "http://example.com",
  "count": undefined,
  "isActive": true
},  {
  "action": "Hello world",
  "url": "http://example.com",
  "count": undefined,
  "isActive": true
}

This works with setting values to undefined too, and avoids having to use the non-null assertion.

[ardeois]

Thanks @emab for the suggestion! I think it's the best approach and it's even cleaner than before

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[emab]

I started looking into this but never actually finished... will see where I got to.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[emab]

Finally got around to this!

[emab]

https://github.com/ardeois/graphql-codegen-typescript-mock-data/pull/131

PR here