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

ReadonlyDeep: Fix handling of objects with call signatures #359

Merged
merged 5 commits into from
Jan 31, 2022

Conversation

RebeccaStevens
Copy link
Contributor

@RebeccaStevens RebeccaStevens commented Jan 29, 2022

partial fix of #337

Makes objects with a call signatures readonly.

Limitations:

Current this PR assumes that any object that has an call signatures is not a map or set. I can extends this PR to handle maps and sets with call signatures.

source/readonly-deep.d.ts Outdated Show resolved Hide resolved
source/readonly-deep.d.ts Outdated Show resolved Hide resolved
source/readonly-deep.d.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Current this PR assumes that any object that has an call signatures is not a map or set. I can extends this PR to handle maps and sets with call signatures.

Do you think it should be handled?

…natures

instead just leave them as be (mutable)
@sindresorhus sindresorhus changed the title fix(readonlydeep): handle case of object with call signatures ReadonlyDeep: Fix handling of objects with call signatures Jan 31, 2022
@RebeccaStevens
Copy link
Contributor Author

RebeccaStevens commented Jan 31, 2022

@sindresorhus It's a rare case but probably should be handled for completeness.

Edit: Actually I don't think it's possible to to make a native map or set callable. Thus this will be a very rare case.

@sindresorhus
Copy link
Owner

It's a rare case but probably should be handled for completeness.

I agree. It would be great if you could handle it then.

@RebeccaStevens
Copy link
Contributor Author

RebeccaStevens commented Jan 31, 2022

@sindresorhus I've made that change locally to support maps and sets but then I realized that with how this type currently works, only native maps and sets are supported.
That is to say if we have the type Set<string> & { customProp: string } the ReadonlyDeep version of this would just be the same as ReadonlyDeep<Set<string>>.

As native map and set can't be made callable (as far as I know), my local change will have no benefit.
If we want to support non-native maps and sets, that should probably be done in it's own PR.

I'd say the PR is currently ready in its current state.

@sindresorhus sindresorhus merged commit db54028 into sindresorhus:main Jan 31, 2022
@sindresorhus
Copy link
Owner

Thanks :)

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