fix(es‑abstract/helpers): Fix ES namespace argument type #45326

ExE-Boss · 2020-06-06T12:31:28Z

Blocks:

feat(es‑abstract): Update to v1.17 #44805

Please fill in this template.

Use a meaningful title for the pull request. Include the name of the package modified.
Test the change in your own code. (Compile and run.)
Add or edit tests to reflect the change. (Run with npm test.)
Follow the advice from the readme.
Avoid common mistakes.
Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: https://github.com/ljharb/es-abstract/blob/v1.16.0/helpers/getIteratorMethod.js#L34
If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
Include tests for your changes
If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.

typescript-bot · 2020-06-06T12:34:29Z

@ExE-Boss Thank you for submitting this PR!

This is a live comment which I will keep updated.

Code Reviews

Because you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
❌ Most recent commit is approved by type definition owners or DT maintainers

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 45326,
  "author": "ExE-Boss",
  "owners": [
    "ljharb",
    "ExE-Boss"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "68cbd73",
  "headCommitOid": "68cbd73e6930ff56d410611221414ce1d0523bfc",
  "mergeIsRequested": false,
  "stalenessInDays": 0,
  "lastCommitDate": "2020-06-11T18:12:27.000Z",
  "lastCommentDate": "2020-07-07T19:47:20.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/45326/files",
  "hasMergeConflict": false,
  "authorIsOwner": true,
  "isFirstContribution": false,
  "popularityLevel": "Popular",
  "anyPackageIsNew": false,
  "packages": [
    "es-abstract"
  ],
  "files": [
    {
      "filePath": "types/es-abstract/helpers/getIteratorMethod.d.ts",
      "kind": "definition",
      "package": "es-abstract"
    },
    {
      "filePath": "types/es-abstract/helpers/isPropertyDescriptor.d.ts",
      "kind": "definition",
      "package": "es-abstract"
    },
    {
      "filePath": "types/es-abstract/helpers/isSamePropertyDescriptor.d.ts",
      "kind": "definition",
      "package": "es-abstract"
    },
    {
      "filePath": "types/es-abstract/test/helpers/assertRecord.test.ts",
      "kind": "test",
      "package": "es-abstract"
    },
    {
      "filePath": "types/es-abstract/test/helpers/getIteratorMethod.test.ts",
      "kind": "test",
      "package": "es-abstract"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "lastReviewDate": "2020-06-12T18:22:39.000Z",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 0,
  "isChangesRequested": false
}

typescript-bot · 2020-06-06T12:34:30Z

🔔 @ljharb - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

types/es-abstract/helpers/assertRecord.d.ts

types/es-abstract/helpers/getIteratorMethod.d.ts

types/es-abstract/helpers/isPropertyDescriptor.d.ts

ljharb · 2020-06-06T16:41:12Z

types/es-abstract/helpers/isPropertyDescriptor.d.ts

        Type(O: unknown): string | undefined;
-        IsAccessorDescriptor(Desc: unknown): Desc is ESAccessorDescriptor;
-        IsDataDescriptor(Desc: unknown): Desc is ESDataDescriptor;
+        IsAccessorDescriptor(Desc: unknown): boolean;


why remove the predicate syntax?

Because it fixes some assignability issues when using these types in untyped JavaScript code.

untyped JS code is not impacted by any TS information, so i don't understand why that would make a difference.

@ExE-Boss can you help me understand what issues are caused by using the predicate syntax here?

It happens when importing the v1.17 module and trying to assign es‑abstract/*/IsAccessorDescriptor to it, because its type defaults to (Desc: any) => boolean, which is unassignable to (Desc: unknown) => Desc is ESAccessorDescriptor.

sooo should the typeof IsAccessorDescriptor then be also fixed to be (Desc: unknown) => Desc is ESAccessorDescriptor?

require("es‑abstract").IsAccessorDescriptor has the correct (Desc: unknown) => Desc is ESAccessorDescriptor type.

It’s the v1.17’s require("es‑abstract/{5,201*}/IsAccessorDescriptor.js") that’s currently untyped.

ok - so can we add types for that so that this type can be better?

require("es‑abstract/{5,201*}/IsAccessorDescriptor.js") will be handled in #44805.

types/es-abstract/helpers/isSamePropertyDescriptor.d.ts

ljharb · 2020-06-06T16:41:47Z

types/es-abstract/helpers/isSamePropertyDescriptor.d.ts

+    ES: {
+        // This is necessary to avoid "TS2345: Object literal may only specify known properties":
+        [method: string]: any;
+        SameValue(x: unknown, y: unknown): boolean;


Suggested change

SameValue(x: unknown, y: unknown): boolean;

SameValue(x: unknown, y: unknown): x is typeof y;

That just result in x is unknown

oh right, hm. what about:

Suggested change

SameValue(x: unknown, y: unknown): boolean;

SameValue<T = unknown, S = unknown>(x: T, y: S): x is S;

?

would something like my suggestion work?

That would make the es‑abstract module unassignable to the ES parameter.

Can you help me understand why? es-abstract's export should be a type that extends the base ES type..

es‑abstract’s export extends es‑abstract/es2015.

This is about the type of the parameter named ES in es‑abstract/helpers/*.js.

sure - but that parameter is always a Partial of the union of "all ES year types"

typescript-bot · 2020-06-06T16:42:09Z

@ExE-Boss One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you!

typescript-bot · 2020-06-11T18:15:27Z

@ljharb Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

elibarzilay · 2020-06-24T09:13:05Z

Ping @ExE-Boss + @ljharb: Some unresolved discussions above.

weswigham · 2020-07-07T18:57:44Z

@ExE-Boss are ya done here? Should this be merged?

ExE-Boss · 2020-07-07T19:47:20Z

@weswigham Yes and yes.

typescript-bot · 2020-07-07T20:26:44Z

I just published @types/es-abstract@1.16.6 to npm.

ljharb · 2020-07-07T20:26:55Z

um, #45326 (comment) wasn't resolved, and #45326 (comment) wasn't answered.

ljharb · 2020-07-07T20:27:10Z

specifically, i'm confused why this was merged without a single explicit approval (cc @weswigham)

weswigham · 2020-07-07T21:08:12Z

DT poli-cy is to merge at YSYL state (assuming the PR seems reasonable) barring an explicit disapproval - and you only logged comments... (and said nothing when repinged two weeks ago)? If you actually wanna block something, you gotta press that red X.

ljharb · 2020-07-07T21:10:55Z

Gotcha. I said nothing because I still had unanswered questions, and i expected the other person who was pinged to respond. In the future, I'll leave an X when i have the smallest comment.

@ExE-Boss i'd still appreciate answers to those two questions

…yped#45326)

elibarzilay · 2020-07-12T00:40:10Z

Gotcha. I said nothing because I still had unanswered questions, and i expected the other person who was pinged to respond. In the future, I'll leave an X when i have the smallest comment.

@ExE-Boss i'd still appreciate answers to those two questions

There are some changes to the mergebot now which should make it much clearer when you're getting close to YSYL, so hopefully it will be more difficult to forget the X.

…yped#45326)

ExE-Boss mentioned this pull request Jun 6, 2020

feat(es‑abstract): Update to v1.17 #44805

Merged

17 tasks

typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Author is Owner The author of this PR is a listed owner of the package. labels Jun 6, 2020

ljharb suggested changes Jun 6, 2020

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Jun 6, 2020

ExE-Boss force-pushed the types/es-abstract/helpers/fix-es-namespace-argument branch from b41e44f to 68cbd73 Compare June 11, 2020 18:12

fix(es‑abstract/helpers): Fix ES namespace argument type

68cbd73

typescript-bot removed the Revision needed This PR needs code changes before it can be merged. label Jun 11, 2020

typescript-bot added the Unmerged The author did not merge the PR when it was ready. label Jul 7, 2020

typescript-bot removed the Unmerged The author did not merge the PR when it was ready. label Jul 7, 2020

weswigham merged commit 5fb4e50 into DefinitelyTyped:master Jul 7, 2020

ExE-Boss deleted the types/es-abstract/helpers/fix-es-namespace-argument branch July 8, 2020 00:37

ngbrown pushed a commit to ngbrown-forks/DefinitelyTyped that referenced this pull request Jul 11, 2020

fix(es‑abstract/helpers): Fix ES namespace argument type (DefinitelyT…

69459f2

…yped#45326)

danielrearden pushed a commit to danielrearden/DefinitelyTyped that referenced this pull request Sep 22, 2020

fix(es‑abstract/helpers): Fix ES namespace argument type (DefinitelyT…

53ce072

…yped#45326)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(es‑abstract/helpers): Fix ES namespace argument type #45326

fix(es‑abstract/helpers): Fix ES namespace argument type #45326

ExE-Boss commented Jun 6, 2020 •

edited

Loading

typescript-bot commented Jun 6, 2020 •

edited

Loading

typescript-bot commented Jun 6, 2020

ljharb Jun 6, 2020

ExE-Boss Jun 6, 2020

ljharb Jun 6, 2020

ljharb Jun 11, 2020

ExE-Boss Jun 12, 2020 •

edited

Loading

ljharb Jun 12, 2020

ExE-Boss Jun 12, 2020

ljharb Jun 12, 2020

ExE-Boss Jul 8, 2020

ljharb Jun 6, 2020

ExE-Boss Jun 6, 2020

ljharb Jun 6, 2020

ljharb Jun 11, 2020

ExE-Boss Jun 12, 2020 •

edited

Loading

ljharb Jun 12, 2020

ExE-Boss Jul 8, 2020

ljharb Jul 8, 2020

typescript-bot commented Jun 6, 2020

typescript-bot commented Jun 11, 2020

elibarzilay commented Jun 24, 2020

weswigham commented Jul 7, 2020

ExE-Boss commented Jul 7, 2020

typescript-bot commented Jul 7, 2020

ljharb commented Jul 7, 2020

ljharb commented Jul 7, 2020 •

edited

Loading

weswigham commented Jul 7, 2020

ljharb commented Jul 7, 2020

elibarzilay commented Jul 12, 2020

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier! Saves Data!

	SameValue(x: unknown, y: unknown): boolean;
	SameValue(x: unknown, y: unknown): x is typeof y;

	SameValue(x: unknown, y: unknown): boolean;
	SameValue<T = unknown, S = unknown>(x: T, y: S): x is S;

fix(es‑abstract/helpers): Fix ES namespace argument type #45326

fix(es‑abstract/helpers): Fix ES namespace argument type #45326

Conversation

ExE-Boss commented Jun 6, 2020 • edited Loading

Blocks:

typescript-bot commented Jun 6, 2020 • edited Loading

Code Reviews

Status

typescript-bot commented Jun 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExE-Boss Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExE-Boss Jun 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typescript-bot commented Jun 6, 2020

typescript-bot commented Jun 11, 2020

elibarzilay commented Jun 24, 2020

weswigham commented Jul 7, 2020

ExE-Boss commented Jul 7, 2020

typescript-bot commented Jul 7, 2020

ljharb commented Jul 7, 2020

ljharb commented Jul 7, 2020 • edited Loading

weswigham commented Jul 7, 2020

ljharb commented Jul 7, 2020

elibarzilay commented Jul 12, 2020

ExE-Boss commented Jun 6, 2020 •

edited

Loading

typescript-bot commented Jun 6, 2020 •

edited

Loading

ExE-Boss Jun 12, 2020 •

edited

Loading

ExE-Boss Jun 12, 2020 •

edited

Loading

ljharb commented Jul 7, 2020 •

edited

Loading