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

Object.freeze overloads force the type to be recognized as an array (by the order of overloads) #49149

Open
aghArdeshir opened this issue May 17, 2022 · 8 comments
Labels
Experimentation Needed Good First Issue Help Wanted Suggestion
Milestone

Comments

@aghArdeshir
Copy link
Contributor

@aghArdeshir aghArdeshir commented May 17, 2022

lib Update Request

Configuration Check

My compilation target is es5 and my lib is ["dom", "dom.iterable", "esnext"].
(Which I believe does not matter in this context as the definition of freeze is inside lib.es5.d.ts solely.)

Missing / Incorrect Definition

Object.freeze
image
It is defined in a way that in IDEs it is always supposed the input is an array.
image

Sample Code

Object.freeze<{someKey: 'someValue'}>({/* the problem is with autocomplete here (in IDEs) */});

Documentation Link

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze


I believe the overload that forces arrays is defined in this PR:
https://github.com/microsoft/TypeScript/pull/12434/files

And I believe just removing it will suffice (and the user of Object.freeze needs to specify its an array in case its an array)


If it is confirmed, I would be happy to create the PR for it.

@Josh-Cena
Copy link

@Josh-Cena Josh-Cena commented May 17, 2022

That seems to be an unusual development workflow though. Most of the time you only need to specify Object.freeze({ someKey: "someValue" }) without duplicating the type argument.

@aghArdeshir
Copy link
Contributor Author

@aghArdeshir aghArdeshir commented May 17, 2022

@Josh-Cena I have a SocialNetwork type:

type SocialNetwork = { name: string ; url: string; }

I need this type to be a kinda centric type and reusable. So in different places I can have different functions that produce this structure:

function getFacebook() {
  return Object.freeze<SocialNetwork>({});
}

when I write the function above, I expect my IDE to help me completing the object with suggesting required keywords. And later:

function getFacebook() {
  return Object.freeze<SocialNetwork>({name: 'Facebook', url: 'https://facebook.com/my-page' });
}

when I already have function above, I want any change in future to the actual SocialNetwork type, trigger errors all over the place, so I can easily refactor my code (e.g. change url -> URL) or add new keys and immediately know where to change (e.g. add a new key: displayName: 'My Page')

@Josh-Cena
Copy link

@Josh-Cena Josh-Cena commented May 17, 2022

I see. That makes sense.

@MartinJohns
Copy link
Contributor

@MartinJohns MartinJohns commented May 17, 2022

When you use an explicit type annotation you get much better support:

function getFacebookExplicit(): Readonly<SocialNetwork> {
  return Object.freeze({  });
}

function getFacebookInferred() {
  return Object.freeze<SocialNetwork>({ });
}

image
instead of
image

image
instead of
image

@aghArdeshir
Copy link
Contributor Author

@aghArdeshir aghArdeshir commented May 17, 2022

@MartinJohns thanks 👍 That's actually very good. I agree with most parts.

Actually note that:
image

In the above screenshot you attached, the url is not autocompleted, IDE cannot suggest keys of the objects you pass into Object.freeze:
image

Also, from clean-code point of view and architecture point-of-view what you say is basically right, but the getFacebook is just an example.

The reason I opened this issue is to solve more complicated problems, e.g.:

type SocialNetwork = {
  url: string;
};

type CommonProfileHandle = {
  handle: string;
};

function getSocial(): SocialNetwork | CommonProfileHandle | null {
  if (someConditionIsCorrect) {
    return Object.freeze({
      /* here it is not deterministic */
    });
  } else if (someOtherConditionIsCorrect) {
    return Object.freeze({
      /* here it is not deterministic */
    });
  }

  return null;
}

Update
The thing is that Inferred types and Generic types are there for a reason and sometimes they needs to be used. So this issue is about those cases that people find themselves needing to use Generic Type on Object.freeze.

Update
Another thing is that explicit type annotation does not support automatic refactor (url -> URL)

@andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented May 31, 2022

I think it probably makes sense to move overloads with no constraints on the type parameter after overloads that do have constraints. But even then, you can’t disambiguate a call with explicit type arguments between freeze<T>(o: T): Readonly<T> and freeze<T>(a: T[]): readonly T[]. This is a general limitation of overloads.

Accepting a PR so we can try this out and run extended tests to see if real-world usage breaks. No guarantee it will be mergeable.

@andrewbranch andrewbranch added Suggestion Help Wanted Good First Issue Experimentation Needed labels May 31, 2022
@andrewbranch andrewbranch added this to the Backlog milestone May 31, 2022
@aghArdeshir
Copy link
Contributor Author

@aghArdeshir aghArdeshir commented Jun 1, 2022

Thanks @andrewbranch . Great news. I will try to see if I can understand this whole TypeScript thing and make a change.

(this does not mean I am reserving this issue to work on this)

@andrewbranch
Copy link
Member

@andrewbranch andrewbranch commented Jun 1, 2022

And I believe just removing it will suffice (and the user of Object.freeze needs to specify its an array in case its an array)

I missed this part of the suggestion by the way, but unless I’m missing something, I think you’re right. The array-to-readonly-array overload seems unnecessary, as it’s covered by freeze<T>(o: T): Readonly<T>.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experimentation Needed Good First Issue Help Wanted Suggestion
Projects
None yet
Development

No branches or pull requests

4 participants