Fix deep properties of PackageJson and TsConfigJson

[zardoy]

All top-level properties are marked as optional. This makes sense and with TS defaults it is possible to assign undefined:

const package: PackageJson = {
    bin: undefined,
}

But not when it comes to Record<string, string>

const package: PackageJson = {
    bin: {
        path: undefined // it is required by design, but should be optional
    },
}

I've just replaced Record types with Partial<Record>, because there is no short way of doing that

Maybe we should introduce OptionalRecord type or rewrite everything as {[k: string]?: type}?

The worst thing happens in scripts property. Despite of having ? in properties, we can't specify undefined e.g.

const b: PackageJson = {
    scripts: {
        install: undefined
    }
}

Even VSCode displays that it's possible to assign undefined here, weird.

---

Also, I can't see strict errors in ide for test files.
Why test-d was excluded from tsconfig?

P.S. I understand that as a workaround I could use PartialDeep<PackageJson>, but it wouldn't fix individual types such as Dependency.

[zardoy]

Also, maybe it's time to drop ;? I'm just using prettier config that hates semicolons, love it so much.

[voxpelli]

This is somewhat related to #79 and the fact that both PackageJson and TsConfigJson types should both really be valid JsonValue types, as else they are kind of lying about being Json types.

And as can be seen in the JsonValue definition, undefined is not a valid value there:

type-fest/source/basic.d.ts

Lines 15 to 45 in 6b6d81c

	/**
	Matches a JSON object.
	This type can be useful to enforce some input to be JSON-compatible or as a super-type to be extended from. Don't use this as a direct return type as the user would have to double-cast it: `jsonObject as unknown as CustomResponse`. Instead, you could extend your CustomResponse type from it to ensure your type only uses JSON-compatible types: `interface CustomResponse extends JsonObject { … }`.
	@category Basic
	*/
	export type JsonObject = {[Key in string]?: JsonValue};
	/**
	Matches a JSON array.
	@category Basic
	*/
	export type JsonArray = JsonValue[];
	/**
	Matches any valid JSON primitive value.
	@category Basic
	*/
	export type JsonPrimitive = string | number | boolean | null;
	/**
	Matches any valid JSON value.
	@see `Jsonify` if you need to transform a type to one that is assignable to `JsonValue`.
	@category Basic
	*/
	export type JsonValue = JsonPrimitive | JsonObject | JsonArray;

I guess it can be exemplified by this:

const value = {
	xyz: undefined
};

if (value.xyz === undefined) {
	console.log('value.xyz is undefined!');
}
if (Object.prototype.hasOwnProperty.call(value, 'xyz') === false) {
	console.log('value.xyz property does not exist');
}
if (value.abc === undefined) {
	console.log('value.abc is undefined!');
}
if (Object.prototype.hasOwnProperty.call(value, 'abc') === false) {
	console.log('value.abc property does not exist');
}

Which prints:

value.xyz is undefined!
value.abc is undefined!
value.abc property does not exist

And thus show cases that there's a difference between a property being evaluated as undefined and the object actually having that property.

And the output of JSON.parse can never be like value.xyz in that a property exists, but its value is undefined.

I'm not sure what @sindresorhus thinks here? I'm leaning more towards moving in the direction of #79

[zardoy]

And the output of JSON.parse can never be like value.xyz in that a property exists, but its value is undefined.

I agree, but on the other hand this is the limitation for JSON.stringify.

Just because by TypeScript design there is no difference between property that exist with undefined value and property that doesn't exist, that's why ? accepts both:

const obj: {a?: string} = {a: undefined}

And it makes sense, since undefined values would be omitted from JSON.stringify output (but only in case if they are values of object, not array)

So, I'm using this trick to remove values that come from other sources e.g. I use return { ...deps, tsd: undefined } instead of delete deps.tsd;return deps.

Of course, I can use PartialDeep<PackageJson> as a workaround here.

---

And the output of JSON.parse can never be like value.xyz in that a property exists, but its value is undefined.

is that bad? I didn't explicitly add undefined to types, I just made them optional (and also undefined-assignable by TS design).

[zardoy]

And as can be seen in the JsonValue definition, undefined is not a valid value there:

Sorry, misread your message (I didn't). Some other working examples with undefined:

const a: JsonObject = {
    bugs: undefined
}

or

const a: PackageJson = {
    bugs: {
        email: undefined
    }
}

[voxpelli]

by TypeScript design there is no difference between property that exist with undefined value and property that doesn't exist, that's why ? accepts both:

This was true up until TypeScript 4.4 was recently released.

TypeScript 4.4 introduced --exactOptionalPropertyTypes which no longer infers undefined from optional properties.

Due to #65 it doesn't apply correctly to JsonObject right now unfortunately, which I would consider a bug.

You can see it in more proper action on TS Playground here.

Edit: Fixed the playground link

[voxpelli]

And the output of JSON.parse can never be like value.xyz in that a property exists, but its value is undefined.

is that bad? I didn't explicitly add undefined to types, I just made them optional (and also undefined-assignable by TS design).

I think that as long as PackageJson is a valid JsonValue then I'm fine with it, though until #79 gets fixed that becomes hard to test here.

I guess there might also be a case where we need to ensure that its tested in the --exactOptionalPropertyTypes mode, as that's the strictest one in this regard.

[sindresorhus]

Due to #65 it doesn't apply correctly to JsonObject right now unfortunately, which I would consider a bug.

Can you elaborate? It's unclear to me why #65 is a problem for this pull request? From my perspective, this can be merged, and we can look into fixing #272 later.

[sindresorhus]

@voxpelli Are we good to merge this?

[voxpelli]

@sindresorhus Sorry, I've dropped things completely here I see:

My trail of thought was:

1. PackageJson should be possible to use wherever a JsonObject is requested. That's currently not possible according to PackageJson is not a valid JsonObject #79
2. As Make JsonObject work as expected with TS 4.4 --exactOptionalPropertyTypes #272 points out, { bugs: undefined } is not a possible value of a JsonObject (though it is a valid object when using JSON.stringify(), but will be treated the same as if it was left out completely)
3. Considering Make JsonObject work as expected with TS 4.4 --exactOptionalPropertyTypes #272, this PR would bring us further from PackageJson is not a valid JsonObject #79 than we are today.

Though I now elaborated in #272 (comment) that its like more complex than that, so lets merge this and deal with the #79 and #272 problems in those respective issues.

(As I mentioned in #272, it kind of depends on whether one see the JsonObject/ PackageJson value to represent something that is valid to go into a JSON.stringify() or whether its more strictly something that one could expect back from JSON.parse(), but lets leave that to those issues)

[privatenumber]

Why is this changed?

VSCode doesn't allow undefined to be set:

tsc requires a non-empty array value:

[privatenumber]

Revert PR: #404