rocket-connect / for-emit-of

Turn Node.js Events into Async Iterables.
https://www.npmjs.com/package/for-emit-of
MIT License
9 stars 2 forks source link

Conflict with module transpilation and types declaration #9

Closed Farenheith closed 4 years ago

Farenheith commented 4 years ago

I found some conflict between the module generated and the types declaration on the library. The transpiler generates the exporting like this:

var _default = forEmitOf;
exports.default = _default;
module.exports = exports.default;

The last attribution overwrites the second one and is equivalent to this one:

export = exports.default;

According to TypeScript documentation, modules exported like that must be imported as in the following example:

import forEmitOf = require("for-emit-of");

But the declaration file is generated with the exporting like this:

export default forEmitOf;

Which, according to TypeScript documentation, must be imported like this example:

import forEmitOf from "for-emit-of";

This conflict, depending on the tsconfig.json configuration of the lib user, can cause some compiling error. There is a solution for the user, though!

In compileOptions, it's possible to use this flag:

"compilerOptions": {
    ....
    "esModuleInterop": true
  },

This will allow that TypeScript accepts the default import syntax for both cases. But, the best scenario is that the importing works no matter the tsconfig configuration.

Sugestion

You can use tsc instead of babel for the transpiling, with a tsconfig like this:

{
  "compilerOptions": {
    "rootDir": "src",
    "target": "es2018",
    "module": "commonjs",
    "declaration": true,
    "outDir": "./dist",
    "removeComments": true,
    "strict": false,
    "noImplicitAny": false,
    "forceConsistentCasingInFileNames": true
  },
  "include": ["src"]
}

and a build command like this:

"build": "rm -rf dist && tsc"

The rootDir and include will keep the same dist folder structure of previous building. Notice that I let target as es2018 because it's the last version supported by node 10.x, which is the minimal version declared for this library in the package.json. With this config, the generated module will look like this:

exports.default = forEmitOf;

Which will make the importing always works no matter the project configuration!

But I don't know if this suggestion will keep the behavior you need to as I don't know all the scenarios you have in use.

danstarns commented 4 years ago

@Farenheith nice spot, I was using babel because we are using @babel/plugin-transform-runtime to support syntax. But looking at it, this is not needed as MDN says here >=v10.0.0. This is great because we can remove the 1 dependency we have babel.

Farenheith commented 4 years ago

I'm glad this was useful, then! I think all you have using this lib will keep working with this new transpiling, but maybe is a good thing to be sure that no side effects are generated

danstarns commented 4 years ago

@Farenheith I've made a PR for this, if you have the time, id appreciate another pair of eyes before I release. I'll keep it open for some time... For some reason, I can't add you as a PR reviewer :/