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

Fix typedef #5

Merged
merged 1 commit into from Aug 29, 2018
Merged

Fix typedef #5

merged 1 commit into from Aug 29, 2018

Conversation

qm3ster
Copy link
Contributor

@qm3ster qm3ster commented Aug 29, 2018

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...

Since `module.exports` is actually overwritten by the function returned by `create(Error)`, this is not an es6-module compatible scenario.
With this change,
```ts
import * as ono from 'ono'
throw ono(new Error('ono'))
```
and, perhaps more correctly
```ts
import ono = require('ono')
throw ono(new Error('ono'))
```
typecheck and generate correct code.

Before this change, only
```ts
import ono from 'ono'
throw ono(new Error('ono'))
```
and
```ts
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
Copy link

Coverage Status

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

@qm3ster
Copy link
Contributor Author

qm3ster commented Aug 29, 2018

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

@JamesMessinger
Copy link
Member

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.

@JamesMessinger JamesMessinger merged commit 9296561 into JS-DevTools:master Aug 29, 2018
@qm3ster qm3ster deleted the patch-1 branch August 30, 2018 17:00
@qm3ster
Copy link
Contributor Author

qm3ster commented Aug 30, 2018

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? 🤔 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 }

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

Successfully merging this pull request may close these issues.

None yet

3 participants