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

Additional UnknownRecord.is check for undefined added #664

Merged
merged 1 commit into from
Oct 6, 2022

Conversation

mlegenhausen
Copy link
Contributor

This PR introduces an additional check for the undefined value to UnknownRecord.is. Background is that we at Unsplash experience a "small" amount of Chrome users where Object.prototype.toString.call(undefined) returns [object Window] instead of [object Undefined].

We weren't able to reproduce this error on our side, but are in the process of rolling out a patch to all users.

@gcanti
Copy link
Owner

gcanti commented Oct 6, 2022

@mlegenhausen what do you think about #611 (comment)?

@mlegenhausen
Copy link
Contributor Author

Sounds interesting, still does not explain why the string representation of undefined is sometimes different but it seems we can not rely on Object.prototype.toString at all.

On first sight u !== null && typeof u === 'object' && !Array.isArray(u) seems to be a valid replacement.

If you want I can change my PR accordantly?

@gcanti
Copy link
Owner

gcanti commented Oct 6, 2022

If you want I can change my PR accordantly?

Yeah, looks like we can kill two birds with one stone

@mlegenhausen
Copy link
Contributor Author

Of cause to good to be true that this a drop in replacement 😉 this has false positives for new Number(), new Boolean() and so on.

@mlegenhausen
Copy link
Contributor Author

Looking at the PR 231b9f5 it seems in a previous version new Number was a valid UnknownRecord then it was changed to being invalid. Should we move this case again to being valid?

@gcanti
Copy link
Owner

gcanti commented Oct 6, 2022

Should we move this case again to being valid?

IMO yes, the goal of that PR was to not allow arrays but ended up disallowing more Record<string, unknown> candidates

@mlegenhausen
Copy link
Contributor Author

@gcanti ready for review.

@gcanti gcanti merged commit 4edbeb3 into gcanti:master Oct 6, 2022
@gcanti
Copy link
Owner

gcanti commented Oct 6, 2022

Thank you @mlegenhausen, patch released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants