anylogger

anylogger copied to clipboard

The current way is inaccurate because anylogger.cjs.js, the main entrypoint, uses CommonJS and does not export default. Thus it works only when the library consumer enables esModuleInterop. Most devs have it enabled, but this option is just a workaround for badly written type declarations.

https://github.com/DefinitelyTyped/DefinitelyTyped#a-package-uses-export--but-i-prefer-to-use-default-imports-can-i-change-export--to-export-default:

For an npm package, export = is accurate if node -p 'require("foo")' works to import a module, and export default is accurate if node -p 'require("foo").default' works to import a module.

Thanks for this contribution! I will review it soon

Thanks a lot Jirutka! This looks like great work!

I have one question though... maybe we can improve it further by fixing anylogger so that export default can be used? I am just getting into typescript and the first real thing I did with it was add type definitions to kurly. There, I also made a test.ts file that invokes some methods and I compile that with tsc so that I can actually test the type defs. And there I also noticed and fixed a problem with default exports. I made it so both forms work there now. Have a look at https://github.com/Download/kurly/blob/master/kurly.d.ts:

export as namespace kurly;

import parse from './parse'
import compile from './compile'

export = { parse, compile }
export default { parse, compile }

I wonder about my export as namespace kurly statement? Is it wrong? I notice in your PR you are using it with open and close braces around the whole module, which I don't have. I think the combination of typescript and environments without a module system is probably pretty rare, but still I'd like to get it right.

Let me start by pulling this into a branch on this repo. Then, I'd like to add tsc to this project and include a test for the type defs. Then we can make the next version have good typedefs taht will remain good because we test them and make sure that it can be required and imported and import defaulted.

Thanks for all your support on this!

@jirutka I added/changed some stuff, can you have a look? Now, when you run npm test it will actually try to compile new file test.ts and run the resulting js. I added a tsconfig.json with esModuleInterop: true I tested and now both these forms work:

import anylogger from 'anylogger'

(tested with import anylogger from './')

and

import anylogger = require('./anylogger')

(tested with import anylogger = require('./'))

Also, in anylogger.js I added a line of code:

anylogger.default = anylogger  // <---
export default anylogger

which is needed to make this work (I think??)

Maybe I totally messed this up. Earlier, I had an ES module export config for rollup as well, but I found that webpack would default to it and actually fail on it so I removed it again. I'm still not completely clear on how to export something with rollup in such a way that both

var anylogger = require('anylogger')

and

import anylogger from `anylogger`

work when creating both cjs and esm modules....

Thus it works only when the library consumer enables esModuleInterop.

Mmm I think my 'solution' has the same problem so not sure why I think I fixed anything 😓

If I messed stuff up, please feel free to correct them. I am a total Typescript n00b still. I just thought that at least testing the defs on npm test would be useful. Also I am still very unsure about export as namespace anylogger; vs declare namespace anylogger { ... }...

This option just hides problems with wrong export/import declarations.

Frankly, I'm not sure whether I agree with this. At least, I completely disagree with the decision by the ES6 spec makers to make ES6 default import/Eeport incompatible with commonjs. They also did not start out that way. At first, export default x was equivalent with module.exports = x. Then someone realized that theoretically, there is a problem. What if someone does?

export default null
export x

It cannot be translated to commonjs. It would fail. And yes, that's true. But imho, they traded a theoretical problem for a real problem because they broke backward compatibility with the most used/popular existing module system. And by then, there were already many tools and packages that were using / relying on the fact that the two were equivalent. If you search for 'default export webpack' or default export rollup or default export typescript you find thousands of real issues that arose from the decision that backward compat was less important then the theoretical issues.

Also, frankly, I like require. It's way more powerful than import. At the same time I recognize that import is the future. So I would love to support both. But this seems very difficult. I explicitly don't want to require commonjs users to write var anylogger = require('anylogger').default. That's just... too ugly!

So I'm not sure about badly written... I feel the spec was badly written. And I've got thousands of real world issues backing up my claim. But ok it is what it is. All I want to know now is: how do I configure rollup / typescript / webpack so that it works everywhere? And if possible, how to do that with the default configs for these tools? It seems that esModuleInterop: true is actually the default for Typescript so for me it seems ok to use/rely on that. But again I'm by no means an expert so I don't care and I can appreciate that it would be best if it 'just works' with and without that flag. But how?

For an npm package, export = is accurate if node -p 'require("foo")' works to import a module, and export default is accurate if node -p 'require("foo").default' works to import a module.

I think after the change I made here in anylogger.js, both statements are true. Both

var anylogger = require('anylogger')

and

var anylogger = require('anylogger').default

should now work I think.

So... what do we gain? Because if I write

exports = anylogger
export default anylogger

tsc complains that I need to add esModuleInterop: true because otherwise it tells me that export = anylogger may not be used in combination with export default anylogger... So can we just drop exports = anylogger and keep only export default = anylogger? And would that alleviate the need for esModuleInterop: true?

I feel passionate about (for typescript) supporting import anylogger from 'anylogger' because I feel that that matches most closely to var anylogger = require('anylogger'). And I want that to work because I want people to be able to do

npm uninstall --save debug
npm install --save anylogger
search + replace `require('debug')` => `require('anylogger')`

currently you can do exactly this and migrate from debug to anylogger nearly painlessly.

EDIT:

Also interesting to see the confusion even in the docs of DefinitelyTyped, which I think should be considered experts:

How do I write definitions for packages that can be used globally and as a module?

The TypeScript handbook contains excellent general information about writing definitions, and also this example definition file which shows how to create a definition using ES6-style module syntax, while also specifying objects made available to the global scope. This technique is demonstrated practically in the definition for big.js, which is a library that can be loaded globally via script tag on a web page, or imported via require or ES6-style imports.

https://github.com/DefinitelyTyped/DefinitelyTyped#how-do-i-write-definitions-for-packages-that-can-be-used-globally-and-as-a-module

Hey, it sounds like big.js has exactly the same requirements as I have for anylogger. To be able to be loaded globally via script tag on a web page, or imported via require or ES6-style imports. So let's check how they did it!

ehm... this comment at the bottom of that file says it all:

// If you pull in big.js via a <script> tag, the global symbol 'Big' is automatically defined.
// To let TypeScript know that, add this to your project's global types file, e.g. "types.d.ts":
//
// import BigJs from 'big.js';
// declare global {
//     const Big = BigJs;
//     type Big = BigJs;
// }
//
// There is a way to have TypeScript know to do this automatically (using "export as namespace"),
// but I couldn't get it working correctly.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/big.js/index.d.ts

🤦

This is a bit of a drive-by comment, so just let me know if I should stay out of things or if this isn't helpful....

Tl;dr: as long as you use CommonJS modules, esModuleInterop is necessary to prevent requiring the user to type import * as foo from 'bar'. This isn't an issue about the typings but instead about the JS for which you want TypeScript to generate import code. If you want to consume the CommonJS version, you need to tell TypeScript that (via the esModuleInterop flag), if you instead just imported/exported purely ES Modules you wouldn't need the extra flag.

There are a few things going on here:

1. Module resolution: by setting "module" to "commonjs", we're allowing the use of "./" as a valid import source in test.ts. This is a node.js naming convention and not standard to vanilla Typescript AFAIK. You can see this by changing "module" to something like "es2015" and trying to run it.
2. Module export: This is what we're setting by allowing "module" to be "commonjs". By its nature, this is not an ES6 module (as noted earlier in this thread).
3. Module import: This is why we have to set allowSyntheticDefaultImports / esModuleInterop. By having our exported module be defined in CommonJS format, we will need to either use star-import syntax (import * as anylogger from 'anylogger') or have that auto-generated for us by esModuleInterop. AFAIK setting these flags only makes the compiler generate an extra wrapping namespace and inject the same logic as we'd generate if we had just use import * as … in the first place. See https://stackoverflow.com/questions/56238356/understanding-esmoduleinterop-in-tsconfig-file for a related write-up of what's going on.

All this means that you need esModuleInterop or similar if you are going to want to import CommonJS modules in the ES6 Module syntax. I.e. you can stop using esModuleInterop when you stop using CommonJS-style modules.

Here's a lightly hacky demonstration making everything just use ES6 Modules (though as you're still using Node.js to run the generated code, it still needs "moduleResolution": "node") https://github.com/akesling/anylogger/tree/es-module .

Hope this is of any help.

as long as you use CommonJS modules, esModuleInterop is necessary to prevent requiring the user to type import * as foo from 'bar'

When esModuleInterop is disabled and you import * as foo from 'bar', tsc converts it into const foo = require('bar'), which is correct.

If you want to consume the CommonJS version, you need to tell TypeScript that (via the esModuleInterop flag),

No, you’re wrong. Look into types and linting rules in https://github.com/DefinitelyTyped/DefinitelyTyped.

I’m sorry, I  don’t have time to answer the questions right now (I’ll later).

@akesling Your input is appreciated!

as long as you use CommonJS modules, esModuleInterop is necessary to prevent requiring the user to type import * as foo from 'bar'.

Ok so I'm using rollup to build the commonjs version, named anylogger.cjs.js. I once had a config where I was using rollup to generate an es6 version as well, named anylogger.esm.js. But I disabled this as it was breaking imports via webpack for some reason... Maybe we could solve this issue by generating an es6 module as well like I tried to before?

If you want to consume the CommonJS version, you need to tell TypeScript that (via the esModuleInterop flag),

No, you’re wrong. Look into types and linting rules in https://github.com/DefinitelyTyped/DefinitelyTyped.

I’m sorry, I  don’t have time to answer the questions right now (I’ll later).

@jirutka When you have the time, I would greatly appreciate more information about how I'm misunderstanding. Thank you in advance for helping further my TypeScript education.

I'm already quite familiar with DefinitelyTyped and have read through the TypeScript documentation on types/linting at least once. A more precise link would be appreciated in helping elucidate what I've missed, thank you.

Ok so I'm using rollup to build the commonjs version, named anylogger.cjs.js. I once had a config where I was using rollup to generate an es6 version as well, named anylogger.esm.js. But I disabled this as it was breaking imports via webpack for some reason... Maybe we could solve this issue by generating an es6 module as well like I tried to before?

@Download Maybe that was an issue with the webpack configuration itself? Do you still have a commit of what you were trying somewhere?

No, you’re wrong. Look into types and linting rules in https://github.com/DefinitelyTyped/DefinitelyTyped.

I’m sorry, I  don’t have time to answer the questions right now (I’ll later).

nit: @jirutka to help improve community inclusiveness, here's some feedback on your reply as it could easily be interpreted in a less than productive manner. Please take this reply as the kind-hearted feedback it is intended to be:

FYI, when replying to comments, it's more productive to phrase things in terms of the idea that's wrong as opposed to phrasing things in terms of the person. This helps improve inclusiveness in our community by decreasing ego involvement,. Decreasing ego involvement can decrease defensiveness and help avoid a tendency for discussion to devolve to argument.

Some example rephrasings / context on interpretation:

1. one possible way to more gracefully have phrased "No, you're wrong" would be "This isn't technically true, please take another look at...."
2. replacing "Look into types and linting rules in https://github.com/DefinitelyTyped/DefinitelyTyped" with something like "please take another look at <thing more specific to the issue at hand> in <more precise link>" would better target the issue. As written "look into types and linting rules..." can be easily misconstrued to imply that the original author being replied to knows nothing about TypeScript, especially when the link doesn't directly give any context to the statement provided.

Again, thank you for taking the time to help me/us better understand this. Please take a moment to consider my suggestions.

@akesling It is indeed very hard to have technical discussions and leave the ego at the door. I know I'm guilty as charged. I have a big-ass ego, sorry. But I think we should all try to do both: write in a manner that is about issues and code and ideas, not about people and their ego, and read in that same way. Some people are also from different cultures where there are different customs and ways of saying things making it even harder. I myself am from the Netherlands and we are quitte rude apparently... At least that's what foreign people often seem to think about us 😅

Coming back to the issue at hand... What I wonder about is this:

Is the idea to have CJS code call

var anylogger = require('anylogger')

and have ESM code call

import anylogger from 'anylogger'

fundamentally broken? I think, maybe it is and that's where the confusion comes from?

If it is, we have to come up with a good idea for how to go forward.

I really value the fact that var anylogger = require('anylogger') is so natural for Node JS people and that it's a natural substitute for var debug = require('debug') that people use. It makes migrating a breeze. debug is the de facto logging standard on Node JS, with thousands of packages depending on it. In my ideal world these packages would start to depend on the combination anylogger debug anylogger-debug and decouple their client code from debug. I really want to make that as easy as possible for them.

At the same time I see that ESM modules are the future and I really want that import to be natural as well. I do feel a default export makes a lot of sense for a module where there is basically only a single function being exported. My other module, kurly is different in that sense in that it basically has 2 functions (parse and compile) being exported and the kurly object is just that; an empty container object. So for kurly import * as kurly from 'kurly' makes a lot more sense.

I spent way too much time reading about this already though. And somehow it just doesn't click. There are a lot of things coming together here:

• How to build (a) module(s) in such a way that it is consumable from CJS and ESM alike?
• How to use rollup for this? Or maybe switch bundler or build manually? I like rollup for it's small bundle size but would prefer Webpack otherwise
• What considerations are there for the consuming side? If we use webpack, will it pick the 'right' bundle? The problem I had was that even for 'var anylogger = require('anylogger')' it would pick the ESM bundle if it existed... And then fail on it.
• How does Typescript come into the mix
• Native ESM vs transpiled ESM? If I create an ESM module that imports anylogger and try to run that on Node JS or modern browsers directly, how does that compare to using import in a file I am transpiling with Babel?

For now, for me, the requirement to have esModuleInterop: true, which is the default, is not a big problem for me. But if there is a good way to do without then yes, please. But I don't know how.

@jirutka My edits to your PR were not meant to overrule you. Just to have a discussion about it. Please feel free to roll them back or change them. I want to merge this, but I also feel it is very important to anylogger users that they can feel safe in upgrading from one version to the next. Anylogger should be backwards compatible. That also means that if we screw this up now, we will have to either break clients or stay backward compatible with our mistake. I'd like to avoid both so I'd like to feel assured that whatever solution we choose, it will be good for the next versions as well.

Maybe we should make a mini project with rollup and a single 'helloWorld' function and create CJS and ESM modules for it, then create some example client packages importing it in all kinds of ways to see what actually works well:

• esm-cjs-hello ** hello.esm.js ** hello.cjs.js
• hello-cjs-native ** var helloWorld = require('esm-cjs-hello') ** Run on Node JS directly
• hello-esm-native ** import helloWorld from 'esm-cjs-hello' ** Try on latest Node JS ** Try on browsers (if that makes sense?)
• hello-cjs-transpiled ** var helloWorld = require('esm-cjs-hello') ** Transpile with Babel ** Use with Webpack ** Use with Rollup
• hello-esm-transpiled ** 'import helloWorld from 'esm-cjs-hello'' ** Transpile with Babel ** Use with Webpack ** Use with Rollup
• Typescript projects ** CJS native ** CJS transpiled ** ESM native ** ESM transpiled ** esModuleInterop: true ** esModuleInterop: false
• ...?

This is a lot of work, but I fear that might be needed if we want to do this right. Otherwise we might end up turning early anylogger adopters into guinea pigs...

But I think we should all try to do both: write in a manner that is about issues and code and ideas, not about people and their ego, and read in that same way.

@Download while I agree with this sentiment (the principle of charity is incredibly valuable) it's important that we all strive to not require it in our readers. As a particular example, it's my experience that under-represented minorities in tech tend to already be less confident in their abilities as a function of experiencing cultural bias against them. By striving to be actively inclusive, we can help improve the community for everyone.

My comment in response to @jirutka was an attempt, in context, to help move the needle just a little bit more toward inclusivity. By actively saying something instead of just passively being charitable I hope to contribute to making the community that much better instead of leaving the status quo alone. I think everyone on this thread is well-meaning and I want to help that intent come across as such as much as possible :).

For now, for me, the requirement to have esModuleInterop: true, which is the default, is not a big problem for me. But if there is a good way to do without then yes, please. But I don't know how.

@jirutka might have an idea, but AFAIK except for some magic with conditional exports one can't support both CJS and ESM consumption of the same npm package transparently in Typescript (which I believe to be what our discussion of esModuleInterop: true effectively boils down to). I'm unfamiliar with actually using conditional exports myself though :/, it could very well be an option. There might even be some way to just ship the CJS version now and add the conditional export of ESM later on....

I don't know if the perspective is useful (it's only one datapoint), but, as a user, I have no issue setting that flag. IMHO it seems fine to only provide a CJS version of the package and expect users to pick between import * as anylogger from 'anylogger' and import anylogger from 'anylogger' depending on how they set esModuleInterop.

@jirutka How are you doing? I myself am very busy atm with a new job 😅 Do you feel this PR is still going somewhere? Do you have time in the coming weeks to work on it?

Closing this as @jirutka seems to have given up on it...