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

Drop non-working default export for TypeScript users #89

Closed
wants to merge 9 commits into from
Closed

Drop non-working default export for TypeScript users #89

wants to merge 9 commits into from

Conversation

fregante
Copy link
Contributor

@fregante
Copy link
Contributor Author

I'm not entirely sure of how to fool import/default without having an actual export default

See eslint's log at https://travis-ci.org/sindresorhus/is/jobs/533220117

@sindresorhus
Copy link
Owner

I would prefer just reverting back to:

export default is;

// For CommonJS default export support
module.exports = is;
module.exports.default = is;

Which we know are working.

@fregante
Copy link
Contributor Author

export default is isn't right? sindresorhus/memoize#31

@sindresorhus
Copy link
Owner

Not exactly, since the source here is TypeScript, while sindresorhus/memoize#31 is about CommonJS.

@fregante
Copy link
Contributor Author

fregante commented May 18, 2019

The previous change broke compatibility (sadly, but we’re still v0.x) but given the TODO comment in that PR, I think your modules aren’t meant to export default in the first place.

is is still a CJS module and thus it should not be imported via import is from x in TypeScript, especially with esModuleInterop disabled 🤷‍♂️

@fregante fregante changed the title Restore non-interop ES Modules support Drop non-working default export May 18, 2019
@sindresorhus sindresorhus changed the title Drop non-working default export Drop non-working default export for TypeScript users May 19, 2019
@sindresorhus
Copy link
Owner

You have to update the test to use import is = require('.') syntax.

test/test.ts Outdated
@@ -7,11 +7,15 @@ import test, {ExecutionContext} from 'ava';
import {JSDOM} from 'jsdom';
import {Subject, Observable} from 'rxjs';
import ZenObservable from 'zen-observable';
import is, {TypeName} from '../source';

// eslint-disable-next-line @typescript-eslint/no-require-imports
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either import/default follows esModuleInterop and therefore allows import X from Y on CJS modules, or @typescript-eslint/no-require-imports should be disabled.

Since I have to disable a rule, I might as well leave the old ESM import and disable import/default on that line

@fregante
Copy link
Contributor Author

The last two commits are valid but there's an unrelated TS issue:

  Uncaught exception in test/test.ts
  test/test.ts:30:13 - error TS2749: 'TypeName' refers to a value, but is being used as a type here.
  
  30  typename?: TypeName;
                 ~~~~~~~~

@sindresorhus
Copy link
Owner

Fixing this properly requires too many breaking changes. We'll also have to put every type export behind a namespace. I'm just gonna revert the original commit and keep it like this until we can move to ESM later this year.

@fregante fregante deleted the patch-2 branch May 21, 2019 11:32
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.

Runtime errors if compiled with "esModuleInterop": false
2 participants