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

Refactor away from is.plainObject #22666

Open
JamieMagee opened this issue Jun 10, 2023 · 1 comment
Open

Refactor away from is.plainObject #22666

JamieMagee opened this issue Jun 10, 2023 · 1 comment
Labels
priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:refactor Refactoring or improving of existing code

Comments

@JamieMagee
Copy link
Contributor

Describe the proposed change(s).

We currently use the @sindresorhus/is library to simplify a lot of type checking throughout Renovate. During a refactor from our own clone function to structuredClone in #20885 we saw some issues with some of the limitations of structuredClone. Namely, the prototype chain is not preserved.

After the PR was merged another issue emerged, #22058. The root cause of this is the implementation of is.plainObject is not compatible with use of structuredClone:

is.plainObject = <Value = unknown>(value: unknown): value is Record<PropertyKey, Value> => {
	// From: https://github.com/sindresorhus/is-plain-obj/blob/main/index.js
	if (typeof value !== 'object' || value === null) {
		return false;
	}

	// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
	const prototype = Object.getPrototypeOf(value);

	return (prototype === null || prototype === Object.prototype || Object.getPrototypeOf(prototype) === null) && !(Symbol.toStringTag in value) && !(Symbol.iterator in value);
};

The prototype of the object being checked is compared with the prototype of the Object type. This will always return false with objects that are cloned with structuredClone.

I don't believe this is a bug with @sindresorhus/is, as it's explicitly called out in their documentation:

An object is plain if it's created by either {}, new Object(), or Object.create(null).

Therefore, if we want to continue using structuredClone (we do! See #20553) we need to migrate away from is.plainObject to something else.

The naive choice is is.object. However, the implementation is not exactly what

is.object = (value: unknown): value is object => !is.null_(value) && (typeof value === 'object' || is.function_(value));

And the documentation confirms this:

Keep in mind that functions are objects too.

That only really leaves is.nonEmptyObject (and its counterpart is.emptyObject). Its implementation is:

is.nonEmptyObject = <Key extends keyof any = string, Value = unknown>(value: unknown): value is Record<Key, Value> => is.object(value) && !is.map(value) && !is.set(value) && Object.keys(value).length > 0;

I think this is currently the best path forward, though I've also opened sindresorhus/is#183 to get feedback from upstream

Describe why we need/want these change(s).

is.plainObject is incompatible with structuredClone

@JamieMagee JamieMagee added priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:refactor Refactoring or improving of existing code status:ready labels Jun 10, 2023
@JamieMagee
Copy link
Contributor Author

After some more investigation, I think this is caused by us using @sindresorhus/is 4.x. The implementation of is.plainObject used to be:

is.plainObject = <Value = unknown>(value: unknown): value is Record<PropertyKey, Value> => {
	// From: https://github.com/sindresorhus/is-plain-obj/blob/main/index.js
	if (toString.call(value) !== '[object Object]') {
		return false;
	}

	// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
	const prototype = Object.getPrototypeOf(value);

	return prototype === null || prototype === Object.getPrototypeOf({});
};

Specifically

return prototype === null || prototype === Object.getPrototypeOf({})

is causing the issue. In 5.2.0, this was changed to

return (prototype === null || prototype === Object.prototype || Object.getPrototypeOf(prototype) === null) && !(Symbol.toStringTag in value) && !(Symbol.iterator in value);

I'll try and resolve the specific case of this bug in #22058, but this will ultimately be resolved by #9890

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:refactor Refactoring or improving of existing code
Projects
None yet
Development

No branches or pull requests

2 participants