support util.promisify in Node #292

jeysal · 2020-01-19T23:51:11Z

Purpose (TL;DR) - mandatory

Fixes #223, similar to what we did for jest-fake-timers.
Noticed after starting there's already #227, but that seems stale and I'm not sure what some of the changes are for.
I'll do what I can to incorporate any review comments and get this through! 😅
If there's consensus that storing a global.Promise backup is a good idea, can do that, but would do another PR first, changing this in existing code.

SimenB

Wooo!
Should do setImmediate at the same time: https://nodejs.org/api/timers.html#timers_setimmediate_callback_args

SimenB · 2020-01-20T00:22:22Z

src/lolex-src.js

@@ -21,6 +21,7 @@ function withGlobal(_global) {
        hrtimePresent && typeof _global.process.hrtime.bigint === "function";
    var nextTickPresent =
        _global.process && typeof _global.process.nextTick === "function";
+    var utilPromisify = _global.process && require("util").promisify;


_global.process as a "I am in node" check?

Just be sure we are targetting the right global here ... Should not this strictly be globalObject (as that's where the promisify function will be required from in the runtime)? It's not certain that people will pass a custom global with all of the props of the runtime.

_global.process as a "I am in node" check?

Yeah this seems to be one of the popular ways
https://stackoverflow.com/questions/4224606/how-to-check-whether-a-script-is-running-under-node-js

Just be sure we are targetting the right global here ... Should not this strictly be globalObject (as that's where the promisify function will be required from in the runtime)? It's not certain that people will pass a custom global with all of the props of the runtime.

TBH I don't think I have the necessary knowledge of what the use cases for non-defaultImplementations / the withGlobal function are to judge whether there will be cases where the provided _global looks like a Node environment, but our actual global scope does not have require. However FWIW I think it would be unintuitive to have vastly different conditions for applying e.g. the nextTick and util.promisify special handling, so that's why I'd prefer this.

withGlobal was added for jest support so we can pass in the jsdom global instead of the global env of the test runner.

Should probably be considered a bug that we inject process in that case, but removing it would break all of the process.env.NODE_ENV checks people have, but that's not really relevant here

So then it'll apply to the jest-environment-jsdom case anyway because it has process.
Imagining that I pass in e.g. a DOM global that does not have Node things like process, I would probably want it to not apply the promisify change (even though it could, using the outside require), just like it doesn't apply the nextTick change, I think?

I think inspecting _global is correct - if the global used to instantiate lolex pretends to be from node (by having a process property) we should assume we can require('util'); safely and go with that.

src/lolex-src.js

fatso83 · 2020-01-21T10:40:42Z

src/lolex-src.js

@@ -21,6 +21,7 @@ function withGlobal(_global) {
        hrtimePresent && typeof _global.process.hrtime.bigint === "function";
    var nextTickPresent =
        _global.process && typeof _global.process.nextTick === "function";
+    var utilPromisify = _global.process && require("util").promisify;


Just be sure we are targetting the right global here ... Should not this strictly be globalObject (as that's where the promisify function will be required from in the runtime)? It's not certain that people will pass a custom global with all of the props of the runtime.

jeysal · 2020-02-01T21:07:52Z

Anything more to do here? @fatso83 :)
@SimenB not sure how important the storing global.Promise thing is. Since it's not directly related to this, I'd do a separate PR for it, or we can go without it and see if someone actually runs into an issue? Your call

SimenB · 2020-02-01T21:40:05Z

Anything more to do here?

You haven't addressed "Should do setImmediate at the same time".

we can go without it and see if someone actually runs into an issue?

It has hit people in Jest, which is why we have tests for checking that we still work even if people swap out globals (e.g. the whole reason https://github.com/facebook/jest/blob/master/scripts/babel-plugin-jest-native-globals.js exists. That transform only guards Jest's own source code though, not other libraries). I'm fine with doing it separately, it's nothing to block on

jeysal · 2020-02-01T22:22:14Z

You haven't addressed "Should do setImmediate at the same time".

Right, always miss the comments in the reviews themselves 😅 pushed 👍

It has hit people in Jest

I know, it was more of a "are the people changing globals going to be the same people using lolex fake timers" ;D

jeysal · 2020-02-01T22:24:46Z

(Build error also happens on master, test passes for me locally)

mroderick · 2020-02-05T10:36:55Z

@jeysal I can't see why this branch would fail to build on Circle. Would you mind rebasing it, to see if that might solve the build error in Circle?

jeysal · 2020-02-05T12:07:07Z

done

codecov · 2020-02-05T12:08:43Z

Codecov Report

Merging #292 into master will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #292      +/-   ##
==========================================
+ Coverage   92.61%   92.74%   +0.12%     
==========================================
  Files           1        1              
  Lines         542      551       +9     
==========================================
+ Hits          502      511       +9     
  Misses         40       40

Flag	Coverage Δ
#unit	`92.74% <100%> (+0.12%)`	⬆️

Impacted Files	Coverage Δ
src/fake-timers-src.js	`92.74% <100%> (+0.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e08b136...705af40. Read the comment docs.

jeysal · 2020-02-05T12:12:25Z

worked 🎉

mroderick · 2020-02-05T14:27:55Z

@fatso83 are you happy with the changes made? Is there anything left to do?

jeysal · 2020-02-06T16:13:14Z

(I've put @SimenB's suggestion about capturing global.Promise on my to-do list for a follow up, but this one is complete I think)

fatso83 · 2020-03-23T09:35:43Z

wow, wow, wow, I didn't know I was the party pooper here. Sorry for not seeing this! For the next time, there is absolutely no need to wait for my approval on anything, especially if I don't respond (which might happen when I do "Mark all as read" if the number of dependencybot PRs get too high). You guys are collectively way more knowledgeable about this than me, so merge ahead the next time you see things being addressed 😸

mroderick · 2020-03-24T11:44:01Z

@sinonjs/fake-timers@6.0.1

jshbrntt · 2020-12-14T13:33:41Z

By the way this doesn't work for setTimeout (global) because the setTimeout[util.promisify.custom] custom promisified function is not set correctly there.

I assume that's because this for loop is intent on copying the properties on clock.setTimeout to setTimeout (global).

However, the clock.setTimeout[util.promisify.custom] is not copied.

I monkey patched my test with.

const clock = sinon.useFakeTimers();
setTimeout[util.promisify.custom] = clock.setTimeout[util.promisify.custom];

The for...in loop would copy only string own-properties to installed methods, which would omit util.promisify.custom necessary for util.promisify support on Node as added in sinonjs#292. Instead, copy all own-property descriptors to get both Symbol own-properties and to copy any getters/setters that may be added in the future. Add tests for util.promisify behavior. Fixes: sinonjs#347 Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

SimenB requested review from fatso83 and benjamingr January 20, 2020 00:21

SimenB approved these changes Jan 20, 2020

View reviewed changes

fatso83 requested changes Jan 21, 2020

View reviewed changes

jeysal force-pushed the util-promisify branch from ff09789 to b370ab6 Compare January 21, 2020 11:53

benjamingr approved these changes Jan 21, 2020

View reviewed changes

jeysal force-pushed the util-promisify branch from b370ab6 to 9e86833 Compare February 1, 2020 22:20

support util.promisify in Node

705af40

jeysal force-pushed the util-promisify branch from 9e86833 to 705af40 Compare February 5, 2020 12:07

mroderick requested a review from fatso83 February 5, 2020 14:27

fatso83 approved these changes Mar 23, 2020

View reviewed changes

fatso83 merged commit 1a053b3 into sinonjs:master Mar 23, 2020

kevinoid mentioned this pull request Jan 12, 2021

Support util.promisify on installed functions #347

Closed

kevinoid mentioned this pull request Jan 12, 2021

Copy all property descriptors to installed methods #349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support util.promisify in Node #292

support util.promisify in Node #292

jeysal commented Jan 19, 2020

SimenB left a comment

SimenB Jan 20, 2020

fatso83 Jan 21, 2020

jeysal Jan 21, 2020

SimenB Jan 21, 2020 •

edited

Loading

jeysal Jan 21, 2020

SimenB Jan 21, 2020

fatso83 Jan 21, 2020

jeysal commented Feb 1, 2020

SimenB commented Feb 1, 2020

jeysal commented Feb 1, 2020

jeysal commented Feb 1, 2020

mroderick commented Feb 5, 2020

jeysal commented Feb 5, 2020

codecov bot commented Feb 5, 2020

jeysal commented Feb 5, 2020

mroderick commented Feb 5, 2020

jeysal commented Feb 6, 2020

fatso83 commented Mar 23, 2020

mroderick commented Mar 24, 2020

jshbrntt commented Dec 14, 2020 •

edited

Loading

support util.promisify in Node #292

support util.promisify in Node #292

Conversation

jeysal commented Jan 19, 2020

Purpose (TL;DR) - mandatory

SimenB left a comment

Choose a reason for hiding this comment

SimenB Jan 20, 2020

Choose a reason for hiding this comment

fatso83 Jan 21, 2020

Choose a reason for hiding this comment

jeysal Jan 21, 2020

Choose a reason for hiding this comment

SimenB Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

jeysal Jan 21, 2020

Choose a reason for hiding this comment

SimenB Jan 21, 2020

Choose a reason for hiding this comment

fatso83 Jan 21, 2020

Choose a reason for hiding this comment

jeysal commented Feb 1, 2020

SimenB commented Feb 1, 2020

jeysal commented Feb 1, 2020

jeysal commented Feb 1, 2020

mroderick commented Feb 5, 2020

jeysal commented Feb 5, 2020

codecov bot commented Feb 5, 2020

Codecov Report

jeysal commented Feb 5, 2020

mroderick commented Feb 5, 2020

jeysal commented Feb 6, 2020

fatso83 commented Mar 23, 2020

mroderick commented Mar 24, 2020

jshbrntt commented Dec 14, 2020 • edited Loading

SimenB Jan 21, 2020 •

edited

Loading

jshbrntt commented Dec 14, 2020 •

edited

Loading