Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Promisify breaks resolve #151

Closed
reggi opened this issue Feb 22, 2018 · 9 comments
Closed

Promisify breaks resolve #151

reggi opened this issue Feb 22, 2018 · 9 comments

Comments

@reggi
Copy link

reggi commented Feb 22, 2018

I have an example of tests passing (can't publish at the moment) with require.resolve and with resolve as a callback, but when I wrap resolve in const resolveAsync = Promise.promisify(resolve) for some reason the tests that worked are failing. I've also implemented my own wrapped promise and some of tests pass, and others fail, specifically cases where there's a package.json resolve.

Any ideas why this package is un-promisify-able, and potential ways around it?

@ljharb
Copy link
Member

ljharb commented Feb 22, 2018

What promisify implementation are you using?

Since https://npmjs.com/util.promisify is built into node, that’s really the only implementation that matters.

If resolve can’t promisify by default, we could maybe add a custom promisify symbol.

Does it work with the canonical implementation?

@reggi
Copy link
Author

reggi commented Feb 22, 2018

I've been using Bluebird to promisify, but even an initial check here.

➜  ~ trymodule util.promisify resolve
Gonna start a REPL with packages installed and loaded for you
'util.promisify' was already installed since before!
'resolve' was already installed since before!
Package 'util.promisify' was loaded and assigned to 'util_promisify' in the current scope
Package 'resolve' was loaded and assigned to 'resolve' in the current scope
REPL started...
> require.resolve('./Desktop/jest-example/index.js')
'/Users/treggi/Desktop/jest-example/index.js'
> util.promisify(resolve)('./Desktop/jest-example/index.js').then(console.log)
Promise {
  <pending>,
  domain:
   Domain {
     domain: null,
     _events: { error: [Function: debugDomainError] },
     _eventsCount: 1,
     _maxListeners: undefined,
     members: [] } }
> (node:86567) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Cannot find module './Desktop/jest-example/index.js' from 'internal'
(node:86567) [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.

Running node 9.5

@ljharb
Copy link
Member

ljharb commented Feb 23, 2018

Unhandled promise rejection (rejection id: 1): Error: Cannot find module './Desktop/jest-example/index.js' from 'internal' - it seems like it's properly rejecting. Try adding .catch(e => console.error(e))?

@reggi
Copy link
Author

reggi commented Feb 23, 2018 via email

@ljharb
Copy link
Member

ljharb commented Feb 23, 2018

What happens with a non-promisified version of this module, however?

@reggi
Copy link
Author

reggi commented Feb 23, 2018

@ljharb It works perfectly fine

Let me explain this example:

~ trymodule util.promisify resolve
Gonna start a REPL with packages installed and loaded for you
'util.promisify' was already installed since before!
'resolve' was already installed since before!
Package 'util.promisify' was loaded and assigned to 'util_promisify' in the current scope
Package 'resolve' was loaded and assigned to 'resolve' in the current scope
REPL started...

Regular require.resolve works, and resolves this path.

> require.resolve('./Desktop/jest-example/index.js')
'/Users/treggi/Desktop/jest-example/index.js'

The resolve module callback returns the correct resolved path.

> resolve('./Desktop/jest-example/index.js', (err, value) => console.log(value))
undefined
> /Users/treggi/Desktop/jest-example/index.js

Node 9's native util.promisify returns a promise, calls it's .catch and logs the error.

> util.promisify(resolve)('./Desktop/jest-example/index.js').then(console.log).catch(e => console.log(e))
Promise {
  <pending>,
  domain:
   Domain {
     domain: null,
     _events: { error: [Function: debugDomainError] },
     _eventsCount: 1,
     _maxListeners: undefined,
     members: [] } }
> { Error: Cannot find module './Desktop/jest-example/index.js' from 'internal'
    at /Users/treggi/.trymodule/node_modules/resolve/lib/async.js:61:35
    at load (/Users/treggi/.trymodule/node_modules/resolve/lib/async.js:80:43)
    at onex (/Users/treggi/.trymodule/node_modules/resolve/lib/async.js:105:17)
    at /Users/treggi/.trymodule/node_modules/resolve/lib/async.js:26:73
    at FSReqWrap.oncomplete (fs.js:166:21) code: 'MODULE_NOT_FOUND' }

The util.promisify module returns a promise, calls it's .catch and logs the error.

> util_promisify(resolve)('./Desktop/jest-example/index.js').then(console.log).catch(e => console.log(e))
Promise {
  <pending>,
  domain:
   Domain {
     domain: null,
     _events: { error: [Function: debugDomainError] },
     _eventsCount: 1,
     _maxListeners: undefined,
     members: [] } }
> { Error: Cannot find module './Desktop/jest-example/index.js' from 'internal'
    at /Users/treggi/.trymodule/node_modules/resolve/lib/async.js:61:35
    at load (/Users/treggi/.trymodule/node_modules/resolve/lib/async.js:80:43)
    at onex (/Users/treggi/.trymodule/node_modules/resolve/lib/async.js:105:17)
    at /Users/treggi/.trymodule/node_modules/resolve/lib/async.js:26:73
    at FSReqWrap.oncomplete (fs.js:166:21) code: 'MODULE_NOT_FOUND' }

@reggi
Copy link
Author

reggi commented Feb 24, 2018

Any idea what's going on here @ljharb?

@ljharb
Copy link
Member

ljharb commented Feb 24, 2018

@reggi what happens if you call the promisified resolve with an empty object as the second argument?

@ljharb
Copy link
Member

ljharb commented Feb 24, 2018

After looking into this a bit more, the issue is that the assumption that "promisify will work on everything" is flawed; it won't.

Using util.promisfy.custom, we could provide a custom implementation that would work with the native one (but not with bluebird) - but then this module would have to add a dependency on util.promisify and/or restrict which node versions it runs on.

In other words, I think the proper solution is to wrap resolve in new Promise if you want to promisify it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants