[@types/node] use assertion guards in assert module #42786

G-Rath · 2020-03-03T04:48:21Z

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).

Select one of these and delete the others:

If changing an existing definition:

Provide a URL to documentation or source code which provides context for the suggested changes: assertion functions
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.
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.

I've made a start on this, but it was getting a bit tangly so wanted to push it up for the diff & for some guidance on how to handle a few things :)

Closes #40415

Lets try this on for size...

G-Rath · 2020-03-03T04:49:33Z

If this passes, I'll rebase & adjust the commits, so lets not merge.

I'm interested in the order that typesVersions should be in, b/c it seems that ts3.2 is before ts3.5, but when I had ts3.7 after ts3.5 it didn't work, so now I have no idea what's going on 🤷‍♂

Anyway, @SimonSchick watch this space.

(This works for me locally when I edit it in node_modules, but I have to actually catch a train now so it might fail due to me rushing 😬🤷‍♂)

typescript-bot · 2020-03-03T05:22:31Z

@G-Rath Thank you for submitting this PR!

Because this is a new definition, a DefinitelyTyped maintainer will be reviewing this PR in the next few days once the Travis CI build passes.

In the meantime, if the build fails or a merge conflict occurs, I'll let you know. Have a nice day!

G-Rath · 2020-03-03T05:23:41Z

@typescript-bot yeah yeah I know why. I'll fix it when I'm home

SimonSchick · 2020-03-03T22:04:46Z

@G-Rath you do know you can run those tests locally right?

G-Rath · 2020-03-03T22:10:35Z

@SimonSchick it works 🎉🎉🎉 - the bulk of the tests are passing.

There are at least two failures however, which are weird ones:

2> Error: /home/travis/build/DefinitelyTyped/DefinitelyTyped/types/node/v10/ts3.7/node-tests.ts:20:5
2> ERROR: 20:5  expect  TypeScript@next expected type to be:
2>   null | undefined
2> got:
2>   null
2> 
2>     at /home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/dtslint/bin/index.js:193:19
2>     at Generator.next (<anonymous>)
2>     at fulfilled (/home/travis/build/DefinitelyTyped/DefinitelyTyped/node_modules/dtslint/bin/index.js:6:58)

That doesn't seem right; using assert locally the resulting type is null | undefined, but for now I'm happy to ignore that in favor of the other issue, which I suspect is going to be a massive pain:

Error in adone
Error: /home/travis/build/DefinitelyTyped/DefinitelyTyped/types/adone/test/glosses/std.ts:7:9
ERROR: 7:9  expect  TypeScript@next compile error: 
Assertions require every name in the call target to be declared with an explicit type annotation.

std re-exports assert, and I'm not sure how assertion guards behave in that situation 😬

G-Rath · 2020-03-03T22:12:36Z

@G-Rath you do know you can run those tests locally right?

You beat me to the punch 😂

I do indeed, but the tests are actually passing for the most part (aside from v10 being weird) - the failure is in a single library, and running those tests locally eats up resources like crazy since it's installing so many libraries.

I have tested this out locally against libraries in node_modules, which is why the v10 failure is so weird, but they've definitely now actually passing in general which is great.

G-Rath · 2020-03-03T22:14:54Z

So the v10 failure has gone away, so now it's just failing on the adone types.

I'm going to revist the v10 failure once I've got the build passing, b/c the change I had to make is not required for the other versions, which is super weird (unless I'm missing something special about v10?).

G-Rath · 2020-03-03T22:20:24Z

Ugh so I believe this is caused by microsoft/TypeScript#35004, and renders std.assert completely unusable.

We can fix the test by removing the assertion guard from function internal, but it'll actually happen on any assert function that has a guard.

I'll play around later with seeing if there's a way to have the types for that library do something different in ts3.7, like not use ts3.7 assert, but this is the current blocker from shipping :(

G-Rath · 2020-03-03T22:31:24Z

adone is actually deprecated, so I'm wondering if we can get away with breaking these types, or just hacking a fix along the lines of just copying the whole assert types from @node/types 🤷‍♂

G-Rath · 2020-03-04T02:42:53Z

@SimonSchick

you do know you can run those tests locally right?

Actually running them locally results in an error 😬 :

Error: In package.json: Dependency electron-store not in whitelist.
If you are depending on another @types package, do not add it to a package.json. Path mapping should make the import work.
For namespaced dependencies you then have to add a paths mapping from @namespace/library to namespace__library in tsconfig.json.

G-Rath · 2020-03-04T04:18:16Z

types/adone/glosses/std.d.ts

 import dgram = require("dgram");
 import perf_hooks = require("perf_hooks");

+declare const assert: any;


The only dependent for this package on npm is bloater, which just depends on the first 1000 dependencies on npm, and is an attempt to just publish a really large package by the looks of it.

The adone npm package itself is deprecated, and the github project, while recently updated ~20 days ago, doesn't seem to have the std namespace anymore.

Of the two maintainers of these types, one no longer exists on github, and the other has no active contributions.

I believe it's acceptable in this case to slightly weaken the types of a single package that doesn't appear to be used anymore, vs not having assertion guards in @types/node.

Happy to break this out into another PR if needed.

typescript-bot · 2020-03-09T03:28:52Z

We've gotten sign-off from a reviewer 👏. A maintainer will soon review this PR and merge it if there are no issues. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for contributing to DefinitelyTyped!

types/node/ts3.7/assert.d.ts

G-Rath · 2020-03-19T02:01:52Z

@felixfbecker I can't request a re-review from you :(

typescript-bot · 2020-03-19T02:32:09Z

🔔 @rbuckton - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

felixfbecker

Ship it 🚢 🎉

G-Rath · 2020-04-07T21:51:40Z

@rbuckton any chance of a re-review? this PR is ready to go otherwise :)

syabro · 2020-04-16T13:54:10Z

@rbuckton could you please review it? :)

syabro · 2020-04-21T13:08:10Z

Can it be merged without @rbuckton? I'm not sure he is interested in reviewing it :(

rbuckton · 2020-04-22T02:20:59Z

My only comment was about the order in "typesVersions", which seems to have been addressed.

typescript-bot · 2020-04-22T02:25:11Z

I just published @types/[email protected] to npm.

typescript-bot · 2020-04-22T02:26:21Z

I just published @types/[email protected] to npm.

typescript-bot · 2020-04-22T02:27:10Z

I just published @types/[email protected] to npm.

typescript-bot · 2020-04-22T02:27:34Z

I just published @types/[email protected] to npm.

G-Rath · 2020-04-24T02:46:04Z

@sandersn just if it matters @typescript-bot didn't post here about publishing a release of @types/[email protected] to npm.

felixfbecker · 2020-04-26T15:45:51Z

@G-Rath how can I use these when using TS 3.8.3? If I install @types/[email protected] I still get the void return type on assert() when hovering over assert(), and it doesn't work 🤔

G-Rath · 2020-04-26T20:18:32Z

@felixfbecker unfortunately right now they're not usable because the typeVersions property is being sorted before publishing.

In order for TypeScript to use the new typings, that property has to have it's properties inverted; you can use patch-package to correct this until the types publisher is fixed & the types are published.

@sandersn seemed pretty active about it, so I imagine it'll be fixed "soon" 🙂

justinmchase · 2020-04-28T14:48:12Z

Was the version 12 version not published for any particular reason?

typescript-bot · 2020-04-28T14:48:16Z

@G-Rath Thank you for submitting this PR!

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

🔔 @s0m3on3 @maxveres @microsoft @DefinitelyTyped @jkomyno @a-tarasyuk @alvis @r3nya @btoueg @BrunoScheufler @smac89 @tellnes @Touffy @DeividasBakanas @eyqs @Flarna @Hannes-Magnusson-CK @KSXGitHub @hoo29 @kjin @ajafff @islishude @mwiktorczyk @mohsen1 @n-e @octo-sniffle @galkin @parambirs @eps1lon @SimonSchick @ThomasdenH @WilcoBakker @wwwy3y3 @samuela @kuehlein @j-oliveras @bhongy @chyzwar @trivikr @nguymin4 @yoursunny @qwelias @ExE-Boss @Ryan-Willpower @peterblazejewicz - 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.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
✅ Only a DT maintainer can merge changes without tests

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",
  "pr_number": 42786,
  "author": "G-Rath",
  "owners": [
    "s0m3on3",
    "maxveres",
    "Microsoft",
    "DefinitelyTyped",
    "jkomyno",
    "a-tarasyuk",
    "alvis",
    "r3nya",
    "btoueg",
    "brunoscheufler",
    "smac89",
    "tellnes",
    "touffy",
    "DeividasBakanas",
    "eyqs",
    "Flarna",
    "Hannes-Magnusson-CK",
    "KSXGitHub",
    "hoo29",
    "kjin",
    "ajafff",
    "islishude",
    "mwiktorczyk",
    "mohsen1",
    "n-e",
    "octo-sniffle",
    "galkin",
    "parambirs",
    "eps1lon",
    "SimonSchick",
    "ThomasdenH",
    "WilcoBakker",
    "wwwy3y3",
    "samuela",
    "kuehlein",
    "j-oliveras",
    "bhongy",
    "chyzwar",
    "trivikr",
    "nguymin4",
    "yoursunny",
    "qwelias",
    "ExE-Boss",
    "Ryan-Willpower",
    "peterblazejewicz"
  ],
  "dangerLevel": "MultiplePackagesEdited",
  "headCommitAbbrOid": "c6c2a4c",
  "headCommitOid": "c6c2a4ce6586a86ada902276e3ec9d05c267d3be",
  "mergeIsRequested": false,
  "stalenessInDays": 40,
  "lastCommitDate": "2020-03-19T01:06:21.000Z",
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/42786/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": false,
  "popularityLevel": "Critical",
  "anyPackageIsNew": false,
  "packages": [
    "adone",
    "node"
  ],
  "files": [
    {
      "filePath": "types/adone/glosses/std.d.ts",
      "kind": "definition",
      "package": "adone"
    },
    {
      "filePath": "types/node/assert.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/base.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/index.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/package.json",
      "kind": "package-meta",
      "package": "node"
    },
    {
      "filePath": "types/node/ts3.2/base.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/ts3.2/index.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/ts3.5/base.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/ts3.5/index.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/ts3.7/assert.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/ts3.7/base.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/ts3.7/index.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/ts3.7/node-tests.ts",
      "kind": "test",
      "package": "node"
    },
    {
      "filePath": "types/node/ts3.7/tsconfig.json",
      "kind": "package-meta",
      "package": "node"
    },
    {
      "filePath": "types/node/ts3.7/tslint.json",
      "kind": "package-meta",
      "package": "node"
    },
    {
      "filePath": "types/node/v10/assert.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v10/base.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v10/index.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v10/package.json",
      "kind": "package-meta",
      "package": "node"
    },
    {
      "filePath": "types/node/v10/ts3.2/base.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v10/ts3.2/index.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v10/ts3.7/assert.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v10/ts3.7/base.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v10/ts3.7/index.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v10/ts3.7/node-tests.ts",
      "kind": "test",
      "package": "node"
    },
    {
      "filePath": "types/node/v10/ts3.7/tsconfig.json",
      "kind": "package-meta",
      "package": "node"
    },
    {
      "filePath": "types/node/v10/ts3.7/tslint.json",
      "kind": "package-meta",
      "package": "node"
    },
    {
      "filePath": "types/node/v11/assert.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v11/base.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v11/index.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v11/package.json",
      "kind": "package-meta",
      "package": "node"
    },
    {
      "filePath": "types/node/v11/ts3.2/base.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v11/ts3.2/index.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v11/ts3.7/assert.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v11/ts3.7/base.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v11/ts3.7/index.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v11/ts3.7/node-tests.ts",
      "kind": "test",
      "package": "node"
    },
    {
      "filePath": "types/node/v11/ts3.7/tsconfig.json",
      "kind": "package-meta",
      "package": "node"
    },
    {
      "filePath": "types/node/v11/ts3.7/tslint.json",
      "kind": "package-meta",
      "package": "node"
    },
    {
      "filePath": "types/node/v12/assert.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v12/base.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v12/index.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v12/package.json",
      "kind": "package-meta",
      "package": "node"
    },
    {
      "filePath": "types/node/v12/ts3.2/base.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v12/ts3.2/index.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v12/ts3.7/assert.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v12/ts3.7/base.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v12/ts3.7/index.d.ts",
      "kind": "definition",
      "package": "node"
    },
    {
      "filePath": "types/node/v12/ts3.7/node-tests.ts",
      "kind": "test",
      "package": "node"
    },
    {
      "filePath": "types/node/v12/ts3.7/tsconfig.json",
      "kind": "package-meta",
      "package": "node"
    },
    {
      "filePath": "types/node/v12/ts3.7/tslint.json",
      "kind": "package-meta",
      "package": "node"
    }
  ],
  "otherApprovalCount": 2,
  "ownerApprovalCount": 0,
  "maintainerApprovalCount": 1,
  "hasDismissedReview": false,
  "travisResult": "pass",
  "reviewersWithStaleReviews": [],
  "approvalFlags": 5,
  "isChangesRequested": false
}

typescript-bot · 2020-04-28T14:48:16Z

@G-Rath Everything looks good here. Great job! I am ready to merge this PR on your behalf.
If you'd like that to happen, please post a comment with the exact text

Ready to merge

and I'll merge it the next time I look at this PR.

ackwell · 2020-05-09T12:53:35Z

Per the comment in the other issue, it looks like the above issue has indeed been solved. 12.12.38 and 13.13.5 both contain the non-mangled typesVersion property.

That said, it looks like it's only those two versions, other major node version releases i.e. 10.x.y are stale.

Is there a way a new patch release can be pushed of the appropriate major node versions containing the fix to the property?

typescript-bot added New Definition This PR creates a new definition package. Popular package This PR affects a popular package (as counted by NPM download counts). The Travis CI build failed labels Mar 3, 2020

typescript-bot added Awaiting reviewer feedback and removed The Travis CI build failed labels Mar 3, 2020

G-Rath mentioned this pull request Mar 3, 2020

Assertion functions don't work with wildcard imports microsoft/TypeScript#35004

Closed

typescript-bot added The Travis CI build failed and removed Awaiting reviewer feedback labels Mar 3, 2020

G-Rath changed the title ~~Add asserts to node types~~ [@types/node] use assertion guards in assert module Mar 4, 2020

G-Rath commented Mar 4, 2020

View reviewed changes

typescript-bot added Awaiting reviewer feedback and removed The Travis CI build failed labels Mar 4, 2020

amcasey assigned rbuckton Mar 5, 2020

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

lguychard mentioned this pull request Mar 10, 2020

Fix browser extension e2e tests sourcegraph/sourcegraph-public-snapshot#8910

Merged

felixfbecker reviewed Mar 10, 2020

View reviewed changes

types/node/ts3.7/assert.d.ts Outdated Show resolved Hide resolved

felixfbecker approved these changes Mar 10, 2020

View reviewed changes

typescript-bot added Other Approved This PR was reviewed and signed-off by a community member. Merge:LGTM labels Mar 10, 2020

felixfbecker approved these changes Mar 19, 2020

View reviewed changes

rbuckton approved these changes Apr 22, 2020

View reviewed changes

rbuckton merged commit e910379 into DefinitelyTyped:master Apr 22, 2020

G-Rath deleted the add-asserts-to-node-types branch April 22, 2020 02:54

BarryThePenguin mentioned this pull request Jul 30, 2020

[qunit] use asserts for strict equality assertions #46443

Closed

10 tasks

Semigradsky mentioned this pull request Aug 2, 2024

[node] Update typings to v22.2.0 #70193

Merged

[@types/node] use assertion guards in assert module #42786

[@types/node] use assertion guards in assert module #42786

Uh oh!

Conversation

G-Rath commented Mar 3, 2020

Uh oh!

G-Rath commented Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

G-Rath commented Mar 3, 2020

Uh oh!

SimonSchick commented Mar 3, 2020

Uh oh!

G-Rath commented Mar 3, 2020

Uh oh!

G-Rath commented Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

G-Rath commented Mar 3, 2020

Uh oh!

G-Rath commented Mar 3, 2020

Uh oh!

G-Rath commented Mar 3, 2020

Uh oh!

G-Rath commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

G-Rath Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Mar 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

G-Rath commented Mar 19, 2020

Uh oh!

typescript-bot commented Mar 19, 2020

Uh oh!

felixfbecker left a comment

Choose a reason for hiding this comment

Uh oh!

G-Rath commented Apr 7, 2020

Uh oh!

syabro commented Apr 16, 2020

Uh oh!

syabro commented Apr 21, 2020

Uh oh!

rbuckton commented Apr 22, 2020

Uh oh!

typescript-bot commented Apr 22, 2020

Uh oh!

typescript-bot commented Apr 22, 2020

Uh oh!

typescript-bot commented Apr 22, 2020

Uh oh!

typescript-bot commented Apr 22, 2020

Uh oh!

G-Rath commented Apr 24, 2020

Uh oh!

felixfbecker commented Apr 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

G-Rath commented Apr 26, 2020

Uh oh!

justinmchase commented Apr 28, 2020

Uh oh!

typescript-bot commented Apr 28, 2020

Code Reviews

Status

Uh oh!

typescript-bot commented Apr 28, 2020

Uh oh!

ackwell commented May 9, 2020

Uh oh!

Reviewers

Assignees

G-Rath commented Mar 3, 2020 •

edited

Loading

typescript-bot commented Mar 3, 2020 •

edited

Loading

G-Rath commented Mar 3, 2020 •

edited

Loading

G-Rath commented Mar 4, 2020 •

edited

Loading

G-Rath Mar 4, 2020 •

edited

Loading

typescript-bot commented Mar 9, 2020 •

edited

Loading

felixfbecker commented Apr 26, 2020 •

edited

Loading