JS-DevTools / ono

Wrap errors without losing the original message, stack trace, or properties
https://jstools.dev/ono/
MIT License
106 stars 11 forks source link

Fix typedef #5

Closed qm3ster closed 6 years ago

qm3ster commented 6 years ago

Since module.exports is actually overwritten by the function returned by create(Error), this is not an es6-module compatible scenario. With this change,

import * as ono from 'ono'
throw ono(new Error('ono'))

and, perhaps more correctly

import ono = require('ono')
throw ono(new Error('ono'))

typecheck and generate correct code.

Before this change, only

import ono from 'ono'
throw ono(new Error('ono'))

and

import * as themodule from 'ono'
throw themodule.default(new Error('ono'))

would typecheck, both of which would throw at runtime by trying to call ono.default() or ono.default.syntax(), etc...

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-8.2%) to 87.912% when pulling eb5ae0fd5cdce894700502632a1f21ab05a93a41 on qm3ster:patch-1 into 1a2d265943c867684fef3c7e5c4584374a3df945 on BigstickCarpet:master.

qm3ster commented 6 years ago

Yup, that must be accurate. Replacing 7 characters in the untested typings file with 1 made the coverage 8.2% worse. Okay.

JamesMessinger commented 6 years ago

Interesting. I'm not experiencing the same behavior as you. I think it's because our TypeScript environments are different. I'm using TypeScript v3.0.1, and I have the esModuleInterop flag enabled.

Here's the behavior I'm seeing before your change...

Without your change, and without esModuleInterop

import ono from 'ono'       // ✔ typecheck succeeds
throw ono('booom!')         // ❌ fails at runtime

import ono = require('ono') // ❌ typecheck fails
throw ono('booom!')         // ✔ works at runtime

import * as ono from 'ono'  // ❌ typecheck fails
throw ono('booom!')         // ✔ works at runtime

Without your change, but with esModuleInterop

import ono from 'ono'       // ✔ typecheck succeeds
throw ono('booom!')         // ✔ works at runtime

import ono = require('ono') // ❌ typecheck fails
throw ono('booom!')         // ✔ works at runtime

import * as ono from 'ono'  // ❌ typecheck fails
throw ono('booom!')         // ❌ fails at runtime

And here's the behavior I'm seeing after applying your change...

With your change, and without the esModuleInterop flag

import ono from 'ono'       // ❌ typecheck fails
throw ono('booom!')         // ❌ fails at runtime

import ono = require('ono') // ✔ typecheck succeeds
throw ono('booom!')         // ✔ works at runtime

import * as ono from 'ono'  // ✔ typecheck succeeds
throw ono('booom!')         // ✔ works at runtime

With your change, and with the esModuleInterop flag

import ono from 'ono'       // ✔ typecheck succeeds
throw ono('booom!')         // ✔ works at runtime

import ono = require('ono') // ✔ typecheck succeeds
throw ono('booom!')         // ✔ works at runtime

import * as ono from 'ono'  // ❌ typecheck fails
throw ono('booom!')         // ❌ fails at runtime

Even though we're seeing different behavior, and none of the solutions works for every case, it's clear that your change makes it work in more scenarios than before. Also, with your change, there's no situation where typecheck succeeds and runtime fails or vice-versa. It either works in both or fails in both. That's definitely an improvement.

qm3ster commented 6 years ago

Thank you for that extensive testing, I never considered that exactly that setting does in depth. But it makes perfect sense now: Without esModuleInterop, the module.exports object is the es6 module. So this works:

exports.a='a'
exports.b='b'
exports['default']='default'
import { a }, def from 'module-name'

ditto with

module.exports = {a, b, default: 'default'}

But if you assign something other than a plain object, eg:

module.exports = 1

the only way to access it is to get the whole "module object", like this

import * as wholeModule from 'one'
wholeModule === 1

esModuleInterop does something weird though:

Emit __importStar and __importDefault helpers for runtime babel ecosystem compatibility

Not sure what that does under the hood, but as a result, if your cjs module doesn't have exports.default (or maybe if it doesn't have __esModule=true? :thinking: it will return the whole module.exports as default

TL;DR, roughly speaking:

esModuleInterop: false:

{ a: exports.a, b:exports.b }

esModuleInterop: true:

{ a: exports.a, b:exports.b, default: exports }