asyncjs: add then, catch for promise pipelining
#16871
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merged
|
Changelog. |
|
@timotheecour Approx ETA ?, is doable?, is needed for #12531 🙂 |
|
@juancarlospaco ok, getting back to this, thanks for the ping... |
|
...was just asking, but Ok 🙂 |
timotheecour
force-pushed
the
pr_asyncjs_then_catch
branch
from
349b1be
to
306c930
Compare
|
@juancarlospaco PTAL
also, IMO it's better usually to add review comments in the file view instead of the main PR thread so they remain threaded and so I can mark each of them as individually resolved; maybe except for important comments that should garner more attention from other reviewers |
|
how come CI didn't run in azure? |
timotheecour
changed the title
then, catchthen, catch for promise pipelining
timotheecour
force-pushed
the
pr_asyncjs_then_catch
branch
from
225e3a7
to
43f0f82
Compare
ringabout
reviewed
Feb 19, 2021
ringabout
reviewed
Feb 19, 2021
ringabout
approved these changes
Feb 22, 2021
Araq
reviewed
Feb 24, 2021
timotheecour
added
the
TODO: followup needed
timotheecour
added a commit
to timotheecour/Nim
that referenced
this pull request
Feb 25, 2021
2 tasks
timotheecour
added a commit
to timotheecour/Nim
that referenced
this pull request
Feb 26, 2021
timotheecour
added a commit
to timotheecour/Nim
that referenced
this pull request
Mar 4, 2021
ringabout
pushed a commit
to ringabout/Nim
that referenced
this pull request
Mar 22, 2021
* asyncjs: add then * improve tests, changelog, API * fix cryptic windows error: The parameter is incorrect * address comments
ringabout
pushed a commit
to ringabout/Nim
that referenced
this pull request
Mar 22, 2021
…g futures (nim-lang#17189) * followup nim-lang#16871 asyncjs.then: allow pipelining procs returning futures * rename test files where they belong * fix tests * tests for then with `onReject` callback * rename test file containing fail to avoid messing with grep * address comments * cleanup * un-disable 1 test
ardek66
pushed a commit
to ardek66/Nim
that referenced
this pull request
Mar 26, 2021
* asyncjs: add then * improve tests, changelog, API * fix cryptic windows error: The parameter is incorrect * address comments
ardek66
pushed a commit
to ardek66/Nim
that referenced
this pull request
Mar 26, 2021
…g futures (nim-lang#17189) * followup nim-lang#16871 asyncjs.then: allow pipelining procs returning futures * rename test files where they belong * fix tests * tests for then with `onReject` callback * rename test file containing fail to avoid messing with grep * address comments * cleanup * un-disable 1 test
1 task
metagn
removed
the
TODO: followup needed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
prerequisite for things like
jsfetch#12531note
--unhandled-rejections=strictnode flag is now passed in testament for nodejs tests, so thattests/js/tasyncjs_fail.nimreturns exit code != 0 (and so that this test succeeds). This flag is explained in https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode and is the default starting with node >= 15.0.0with this flag, for nodejs < 15.0.0 (all except freebsd CI), you'd get exit code 0 and this message:
(Use
node --trace-warnings ...to show where the warning was created)(node:72796) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag
--unhandled-rejections=strict(see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 3)(node:72796) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
I'm hiding the new API under
nimExperimentalAsyncjsThen, refsnimExperimentalFoofor experimental modules/APIs timotheecour/Nim#575I'm using this pattern cross reference comments via a GUID-like pattern, eg:
D20210217T221607timotheecour/Nim#596 for cross referencing an explanationlinks
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/catch etc
future work
=> upgrade nodejs; add nimCiSystemInfo on azure/github CI #18003
--passnodemaybe), which we'd use as follows:nim r -b:js --lib:lib -d:nodejs --passnode:--unhandled-rejections=strict tests/js/tasyncjs_fail.nimafter which i can mark expected exitcode as != 0 for
tests/js/tasyncjs_fail.nimit'd be handled here: in compiler/nim.nim near
execExternalProgram(conf, cmdPrefix & output.quoteShell & ' ' & conf.arguments)