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

Use named imports #4

Closed
krlwlfrt opened this issue Oct 23, 2018 · 11 comments
Closed

Use named imports #4

krlwlfrt opened this issue Oct 23, 2018 · 11 comments

Comments

@krlwlfrt
Copy link
Contributor

First of all: thank you very much for the concise and easy to use implementation. In my opionion it's much better than comparable solutions.

I added TypeScript definitions to DefinitelyTyped to be able to use your async pool better and more easily in my TypeScript projects. It was a little tricky because you are not using a named import but are exporting the function directly. This leads to rather weird imports in TypeScript modules.

import asyncPool = require('tiny-async-pool');

If you would use named exports the import would be the normal and standard way:

import {asyncPool} from 'tiny-async-pool';
@rxaviers
Copy link
Owner

What if we export a default entry? Would TypeScript be able to load it using import asyncPool from 'tiny-async-pool';?

In order not to break API, we can export both the function and a default export. Please, let me know if that works for you.

@krlwlfrt
Copy link
Contributor Author

I'm actually not sure if that would fix it.

Additionally I actually brought it up because named imports are more declarative and easier to understand and extend. And API breakage would simply mean a new major release which would not impact current users of version 1.x.

@rxaviers
Copy link
Owner

easier to understand

Debatable because it's subjective :)

easier to extend

Agreed, but then we make a major bump.

I'm actually not sure if that would fix it.

If you are interested in testing this out, one easy test you could do in your end (please) before I change things here is to update your locally installed async-pool package with the following:

diff --git a/dist/node.js b/dist/node.js
index 050965d..58a53d0 100644
--- a/dist/node.js
+++ b/dist/node.js
@@ -8,3 +8,4 @@ if (semver.satisfies(process.version, "<8")) {
 }
 
 module.exports = exports;
+module.exports.default = exports;

@krlwlfrt
Copy link
Contributor Author

With this change I had to change the definition to export default function asyncPool instead of

export = asyncPool;

declare function asyncPool(/* ... *) {
  /* ... */
}

It results in the following import:

import asyncPool from 'tiny-async-pool';

@rxaviers
Copy link
Owner

Let's do that update? I understood it looks better for you on TypeScript, although it's not exactly what you wanted (named exports). Do you want to submit a PR? Thanks

@krlwlfrt
Copy link
Contributor Author

If you'd like it that way - sure!

@rxaviers
Copy link
Owner

Fixed by #6

@krlwlfrt
Copy link
Contributor Author

krlwlfrt commented Nov 1, 2018

Would you reconsider this after checking out the related discucssion at DefinitelyTyped?

@rxaviers
Copy link
Owner

rxaviers commented Nov 1, 2018

@krlwlfrt thanks for the TypeScript integration work you're doing.

It seems like in TypeScript you always have to use export = and import = if dealing with CommonJS module.exports except if --esModuleInterop or -allowSyntheticDefaultImports is used in user end (link), correct?

It's not clear to me what could be reconsidered? Even if using a named export such as module.exports = {asyncPool}, it would still require the use of export =, import =, doesn't it?

That being said, what do you think about this... Currently, package.json "main" points to "dist/node.js" which uses commonJS module exports. We need a separate file using ES modules export (i.e., export default ...) that you can use in TypeScript. Is there a TypeScript way to override the "main" property in package.json?

@krlwlfrt
Copy link
Contributor Author

krlwlfrt commented Nov 2, 2018

No. The section that you're referring to only describes what happens with exports = and exports.default.

Named export would result in a TypeScript definition with export function asyncPool.

This would enable an import like import {asyncPool} from tiny-async-pool`.

As far as I know, there is no option do override the main file in Node.js since TypeScript does support different module systems but no other specifics of the platforms it runs on.

@krlwlfrt
Copy link
Contributor Author

krlwlfrt commented Nov 8, 2018

@rxaviers, I'd open a PR with the appropriate changes if you'd like.

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

No branches or pull requests

2 participants