[express-serve-static-core]: (re)fix listen callback type (#35883)" #47063
Conversation
This reverts commit 34f4572. In that commit, arguments were added to the callback on the `app.prototype.listen` method, which is the same as Node.js server one As Node.js `.listen` callback is never called with arguments and to avoid misleadings, arguments to that callback are removed from types as the source code never emits a callback with arguments
* [express-serve-static-core] Add tests * [express-serve-static-core] listen's callback can take some parameters * [express-serve-static-core] Add a definitions by section
|
@davidlj95 Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through. This is a live comment which I will keep updated. 1 package in this PRCode ReviewsBecause you edited one package and updated the tests (👏), I can help you merge this PR once someone else signs off on it. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 47063,
"author": "davidlj95",
"owners": [
"borisyankov",
"19majkel94",
"kacepe",
"micksatana",
"samijaber",
"aereal",
"JoseLion",
"dwrss",
"andoshin11"
],
"dangerLevel": "ScopedAndTested",
"headCommitAbbrOid": "6d78fce",
"headCommitOid": "6d78fceabe5187f3760933ca85fa6dfe165e49dc",
"mergeIsRequested": true,
"stalenessInDays": 0,
"lastPushDate": "2020-08-26T23:26:20.000Z",
"reopenedDate": "2020-08-26T23:26:14.000Z",
"lastCommentDate": "2020-09-01T08:03:36.000Z",
"maintainerBlessed": true,
"reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/47063/files",
"hasMergeConflict": false,
"authorIsOwner": false,
"isFirstContribution": true,
"popularityLevel": "Critical",
"newPackages": [],
"packages": [
"express-serve-static-core"
],
"files": [
{
"path": "types/express-serve-static-core/express-serve-static-core-tests.ts",
"kind": "test",
"package": "express-serve-static-core"
},
{
"path": "types/express-serve-static-core/index.d.ts",
"kind": "definition",
"package": "express-serve-static-core"
}
],
"hasDismissedReview": false,
"ciResult": "pass",
"lastReviewDate": "2020-09-01T07:58:17.000Z",
"reviewersWithStaleReviews": [],
"approvalFlags": 2,
"isChangesRequested": false
} |
|
🔔 @borisyankov @19majkel94 @kacepe @micksatana @samijaber @aereal @JoseLion @dwrss @andoshin11 — please review this PR in the next few days. Be sure to explicitly select |
typescript-bot
moved this from Needs Author Action
to Needs Maintainer Review
in New Pull Request Status Board
|
👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings. Let’s review the numbers, shall we? Comparison details 📊
It looks like nothing changed too much. I won’t post performance data again unless it gets worse. |
typescript-bot
added
the
Perf: Same
elibarzilay
moved this from Needs Maintainer Review
to Waiting for Code Reviews
in New Pull Request Status Board
typescript-bot
added
Owner Approved
typescript-bot
moved this from Waiting for Code Reviews
to Waiting for Author to Merge
in New Pull Request Status Board
|
@davidlj95 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 saying:
and I'll merge this PR almost instantly. Thanks for helping out! ❤️ (@borisyankov, @19majkel94, @kacepe, @micksatana, @samijaber, @aereal, @JoseLion, @dwrss, @andoshin11: you can do this too.) |
|
Ready to merge |
typescript-bot
moved this from Waiting for Author to Merge
to Recently Merged
in New Pull Request Status Board
|
I just published |
… listen callback type (DefinitelyTyped#35883)" by @davidlj95 This reverts commit 34f4572. In that commit, arguments were added to the callback on the `app.prototype.listen` method, which is the same as Node.js server one As Node.js `.listen` callback is never called with arguments and to avoid misleadings, arguments to that callback are removed from types as the source code never emits a callback with arguments
Description of the type change: DefinitelyTyped/DefinitelyTyped#47063
… listen callback type (DefinitelyTyped#35883)" by @davidlj95 This reverts commit 34f4572. In that commit, arguments were added to the callback on the `app.prototype.listen` method, which is the same as Node.js server one As Node.js `.listen` callback is never called with arguments and to avoid misleadings, arguments to that callback are removed from types as the source code never emits a callback with arguments
The error arg was bogus all along, see DefinitelyTyped/DefinitelyTyped#47063
Description
TL;DR: This reverts commit 34f4572.
The
listencallback arguments argumentIn the mentioned commit, arguments to the callback function argument on the
app.prototype.listenmethod were added. Theapp.prototype.listen, as the docs of express say, is passing arguments directly to Node.jshttpserver.listen. But turns out that since the beginning of Node.js, that callback does not receive anything as arguments:nodeserver types specify this callback without arguments.listenThen, in order to prevent devs think that they will receive arguments in that callback (like for instance an error object) when actually no arguments to that callback are sent, I think it should be better to remove those non existing arguments from the type definitions.
How to catch errors on
listenTo catch errors raised on listening, as the docs say, you have to register a listener for the 'error' event in the created server
https://nodejs.org/api/net.html#net_server_listen
In that callback, you'll have the
errorobject describing what happened as the argumentDiscussion
More context:
34f4572
Credits
Thanks to @tsimbalar for raising this issue in a project which lead to this discovery
Checks
npm test.)npm run lint package-name(ortscif notslint.jsonis present).If changing an existing definition:
tslint.jsoncontaining{ "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.