Skip to content

lib Fix Part 3/6 – Object related methods #50451

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

graphemecluster
Copy link
Contributor

@graphemecluster graphemecluster commented Aug 25, 2022

General Information

PR separated out from #49855, because there might be some members expecting smaller PRs. As mentioned in the comments from the big PR, whether to review a single, large PR or 6 smaller PRs is up to the TypeScript Team to decide. I couldn't have found a better way for this; hopefully this will not bring any trouble to the Team.

This PR partially fixes #49773.
For details and the track list about the changes, please see #49773.

Part 3/6, Object related methods

Fix #31175
Fix #24509 as a side-effect

🟠 Breaking Change

Quoted from #49773:

Per the spec, these methods start with either a ToObject or RequireObjectCoercible operation, which throws a TypeError with a nullish value. Starting from TypeScript 4.8 with #49119, {} now precisely represents any nullish value. Thus, the first parameter of these methods should now be typed {} to prevent runtime errors.

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Aug 25, 2022
@sandersn sandersn self-assigned this Sep 13, 2022
@sandersn
Copy link
Member

@typescript-bot run dt
@typescript-bot test top100
@typescript-bot user test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 13, 2022

Heya @sandersn, I've started to run the parallelized Definitely Typed test suite on this PR at 8b640be. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 13, 2022

Heya @sandersn, I've started to run the diff-based user code test suite on this PR at 8b640be. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Sep 13, 2022

Heya @sandersn, I've started to run the diff-based top-repos suite on this PR at 8b640be. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@sandersn Here are the results of running the user test suite comparing main and refs/pull/50451/merge:

Something interesting changed - please have a look.

Details

graceful-fs

/mnt/ts_downloads/graceful-fs/tsconfig.json

  • [NEW] error TS2339: Property '__proto__' does not exist on type '{}'.
    • /mnt/ts_downloads/graceful-fs/node_modules/graceful-fs/clone.js(6,14)

webpack

tsconfig.json

tsconfig.types.json

  • [NEW] error TS2345: Argument of type '{ url: string; } & Writable<{ chunk: Chunk | undefined; filename: string; contentHash: string | false; }>' is not assignable to parameter of type 'PathData | undefined'.
  • [NEW] error TS2345: Argument of type 'B' is not assignable to parameter of type '{}'.
  • [NEW] error TS2345: Argument of type 'A' is not assignable to parameter of type 'object'.
  • [NEW] error TS2345: Argument of type 'T' is not assignable to parameter of type '{}'.
  • [NEW] error TS2322: Type 'Readonly<Writable<{ _fakeHook: true; }>>' is not assignable to type 'FakeHook<T>'.
  • [MISSING] error TS2345: Argument of type '{ url: string; } & { chunk: Chunk | undefined; filename: string; contentHash: string | false; }' is not assignable to parameter of type 'PathData | undefined'.
  • [MISSING] error TS2322: Type 'Readonly<{ _fakeHook: true; }>' is not assignable to type 'FakeHook<T>'.

@typescript-bot
Copy link
Collaborator

@sandersn Here are the results of running the top-repos suite comparing main and refs/pull/50451/merge:

Something interesting changed - please have a look.

Details

conwnet/github1s

2 of 4 projects failed to build with the old tsc and were ignored

extensions/github1s/tsconfig.json

microsoft/vscode

3 of 54 projects failed to build with the old tsc and were ignored

test/smoke/tsconfig.json

neoclide/coc.nvim

tsconfig.json

  • error TS2739: Type 'Writable<any>' is missing the following properties from type 'FileChange': root, subscription, files

TanStack/query

9 of 12 projects failed to build with the old tsc and were ignored

packages/query-broadcast-client-experimental/tsconfig.lint.json

packages/react-query-devtools/tsconfig.lint.json

vercel/hyper

2 of 3 projects failed to build with the old tsc and were ignored

tsconfig.json

  • error TS2345: Argument of type '{ cwd: string; splitDirection: undefined; shell: string; shellArgs: string[]; } & Writable<any> & Writable<{ uid: string; }>' is not assignable to parameter of type 'SessionOptions'.

vercel/swr

3 of 10 projects failed to build with the old tsc and were ignored

_internal/tsconfig.json

core/tsconfig.json

immutable/tsconfig.json

infinite/tsconfig.json

mutation/tsconfig.json

test/tsconfig.json

test/type/tsconfig.json

@graphemecluster graphemecluster changed the title lib Types and Documentations Fix, Part 3/6 lib Fix Part 3/6 – Object related methods Oct 9, 2022
@graphemecluster
Copy link
Contributor Author

Great, TypeScript bug discovered! (Temporarily fixed with a non-null assertion)

Comment on lines +228 to +258
isSealed(o: object): boolean;

/**
* Returns true if existing property attributes cannot be modified in an object and new properties cannot be added to the object.
* @param o Object to test.
*/
isSealed(o: any): true;

/**
* Returns true if existing property attributes and values cannot be modified in an object, and new properties cannot be added to the object.
* @param o Object to test.
*/
isFrozen(o: object): boolean;

/**
* Returns true if existing property attributes and values cannot be modified in an object, and new properties cannot be added to the object.
* @param o Object to test.
*/
isFrozen(o: any): boolean;
isFrozen(o: any): true;

/**
* Returns a value that indicates whether new properties can be added to an object.
* @param o Object to test.
*/
isExtensible(o: any): boolean;
isExtensible(o: object): boolean;

/**
* Returns a value that indicates whether new properties can be added to an object.
* @param o Object to test.
*/
isExtensible(o: any): false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are not actually correct, as object is a subtype of any, and thus, a variable that is a union of object and non‑object will cause the second overload to be selected.

@wchargin
Copy link

Hi, is it possible to revisit this stalled PR, specifically for Object.hasOwn? I have some callers that would like to pass o: string | SomeObjectType, which is sound and would be allowed by this PR but is not allowed as of v5.4.5. I see that there was an objection to changes to the is* methods; could we maybe separate out the other, noncontroversial changes into their own PR so that they can be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Status: Waiting on reviewers
5 participants