add named export

[d-fischer]

This duplicates the default export as a named export.

Why?

It is currently impossible to use this in a TypeScript module that compiles to both CJS and ESM targets from the same source code. This is because of the way default imports are handled.

In a CJS target, only this works (and has the TypeScript compiler complain):

import * as klona from 'klona';

Whereas in an MJS target, only this works:

import klona from 'klona';

The respective other one will fail at runtime.

With this PR, this will work for both targets:

import { klona } from 'klona';

The default export is kept for backwards compatibility, although I'd advise to get rid of it in a future major update.

Please note that your build system currently doesn't seem to support this, as building the output files failed for me. I have sent the PR lukeed/bundt#3 to fix this. The future update to that package will need to be incorporated into this PR as well.

[d-fischer]

@lukeed The linked commit actually does NOT fix this.

Now you removed the TS error for the first case, but created one for the second case.

[lukeed]

This was a breaking change, and not something I want -- sorry. It's intentionally a default export, which means that this is/was the correct variant:

import klona from 'klona';

You should check your TS config. You probably want esModuleInterop and/or allowSyntheticDefaultImports enabled. The latter has no code changes, just alters the type-checking stage.

In any event, your TS was loading the file incorrectly (adding a default key that wasn't there). Using TS alongside Rollup would have never given you grief, even without the above settings. Although, if you really wanted to force the CommonJS variant, this is how you could do that:

import klona = require('klona');

Either way, I've updated the type definitions so that it works regardless of how you decide to import the module.

[lukeed]

@d-fischer Both of these are absolutely incorrect:

import * as klona from 'klona';
import { klona } from 'klona';

I know for a fact that this new module definition works, since it's how DefinitelyTyped defines and ships modules, and I've been using this variant for months now across my projects, with my own & external modules.

The previous definition failed when using CommonJS format (require) because it was defined only with ESM/module spec.

[d-fischer]

This was a breaking change

It was not. I added a new form of export; I didn't remove the old one.

You should check your TS config. You probably want esModuleInterop and/or allowSyntheticDefaultImports enabled

I don't, because I'm writing a library and enabling that for my library spreads the need to use that setting to my users as well.

import klona = require('klona');

Doesn't work with ESM.

Both of these are absolutely incorrect:

That's right. The PR made the latter one correct (in addition to keeping what already exists).

[lukeed]

Doesn't work with ESM.

It's not supposed to.

It was not. I added a new form of export; I didn't remove the old one.

It is. It's changing the export signature. While import klona still works, it's imported with a klona.klona circular reference.

The PR made the latter one correct

This PR makes them both viable. The import * as means to import an entire module as an object. The import { key } is importing a piece of that object. Technically, they're both wrong since it's a default ("single") export of a function.

[d-fischer]

It's not supposed to.

Why not? I'm building my own library that's also supposed to work for both ESM and CJS consumers, and I am not happy with a solution that requires me to write duplicate code just in order to import packages differently. Don't tell me you would - that's why you created bundt, right?

it's imported with a klona.klona circular reference.

What's bad about that? It's basically just adding a "feature" (even though that nobody would want to use it, and it can be considered an implementation detail). What would it break?

In any case, while I really like this package, the current state of it requires me to keep maintaining a fork.

[d-fischer]

I would actually have been fine with keeping this closed as long as the PR lukeed/bundt#3 would have been merged (and this package rebuilt with it), which would allow everyone to use the import they want to use. I'm updating it to resolve the introduced conflicts as we speak.

But with the referenced commit from today, a regular default import from TS would unfortunately also be broken.

[lukeed]

import foo = require('foo') is TypeScript's way to exclusively load a CommonJS module. Talk to them about that.

The circular reference is (1) circular, (2) affects function identity, and (3) deoptimizes the function itself.

Please feel free to send a reproduction of the issue(s) you're having. I've written a dozen libraries (not apps) on top of klona and it works fine as is. As mentioned, only type inference/IntelliSense failed when using require('klona') since the definitions said there's no CommonJS format.

On top of that, I've used this definition format a couple dozen times, across modules and libraries, and I know it stands up without a problem. I obviously wouldn't keep doing it if it didn't work.

---

Re: bundt PR. I'll accept the export { foo } addition, but not in it's current state. The inclusion of module.exports.default is not something I'll accept for reasons touched upon here. It's NOT the same thing.

[lukeed]

regular default import from TS would unfortunately also be broken.

Again, check your configuration. This is not true.

[d-fischer]

Alright, I have spun up a test repo for this: https://github.com/d-fischer/klona-ts-test

Output:

$ yarn build:cjs
yarn run v1.22.4
$ tsc
src/default.ts:1:8 - error TS1259: Module '"klona"' can only be default-imported using the 'esModuleInterop' flag

1 import klona from 'klona';
         ~~~~~

  node_modules/klona/klona.d.ts:3:2
    3  export = klona;
       ~~~~~~~~~~~~~~~
    This module is declared with using 'export =', and can only be used with a default import when using the 'esModuleInterop' flag.

src/star.ts:1:24 - error TS2497: This module can only be referenced with ECMAScript imports/exports by turning on the 'esModuleInterop' flag and referencing its default export.

1 import * as klona from 'klona';
                         ~~~~~~~


Found 2 errors.

error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

$ yarn build:esm
yarn run v1.22.4
$ tsc --project tsconfig.esm.json
src/default.ts:1:8 - error TS1259: Module '"klona"' can only be default-imported using the 'allowSyntheticDefaultImports' flag

1 import klona from 'klona';
         ~~~~~

  node_modules/klona/klona.d.ts:3:2
    3  export = klona;
       ~~~~~~~~~~~~~~~
    This module is declared with using 'export =', and can only be used with a default import when using the 'allowSyntheticDefaultImports' flag.

src/require.ts:1:1 - error TS1202: Import assignment cannot be used when targeting ECMAScript modules. Consider using 'import * as ns from "mod"', 'import {a} from "mod"', 'import d from "mod"', or another module format instead.

1 import klona = require('klona');
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/star.ts:1:24 - error TS2497: This module can only be referenced with ECMAScript imports/exports by turning on the 'allowSyntheticDefaultImports' flag and referencing its default export.

1 import * as klona from 'klona';
                         ~~~~~~~


Found 3 errors.

error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

As you can see, the CJS build only works with the require form, and the ESM build works with none.

Please recommend a change to the configuration on this repo to make one of these files compile under both configurations.

Do not recommend allowSyntheticDefaultImports or esModuleInterop, as these are - as I already mentioned - viral and force people using my library to use it as well, possibly making a lot of their other existing imports compile errors.

(For the record: default.ts would compile in both cases with allowSyntheticDefaultImports. But that's not a solution for me.)

[hakimio]

Fully agree with @d-fischer: forcing people to use allowSyntheticDefaultImports is a terrible idea. The library should just work with default TS configuration.

[lukeed]

All you need is esModuleInterop and both configurations will work with default.ts.

Your other examples:

// This NOT supposed to work, delete
import * as klona from 'klona';

// This only CAN work with CommonJS
import klona = require('klona');

.. so they're moot examples.

Again - if you import the new type definitions file into [email protected], and add esModuleInterop -- it works and doesn't require consumers change their configuration to use it. By default, TypeScript flatout can't understand a module supporting both CommonJS and ESM. By default, it expects you to only be working with one – or be working with named exports only. That's why esModuleInterop exists, so that it can be told to actually determine what you're working with, instead of relying on static analysis.

Effectively, what TS needs to support a dual-format module (with default exports) is this:

declare module 'foo' {
  function bar(x: string): string;
  export default bar;
  export = bar;
}

But that throws because it doesn't know why two formats exist.
There's rumor of supporting this better while native exports map is being worked on, but I've no idea where that will actually lead.

// ---

We can move from default to { klona } exports for the next major version. I'm fine with doing that as a breaking change (and highlighting it) instead of shipping worse code just for the sake of it. I'm not quite ready for 2.0 yet, but am working towards it now.

[d-fischer]

Again, these two configuration switches are not solutions because they're viral.

Essentially, this package is useless for TypeScript library developers that don't want to force their users to use specific configuration that is potentially incompatible with their existing code base.

[lukeed]

I understand that. You're taking TS frustrations out on me, when I'm at the mercy of TS design decisions (as are they now). The esModuleInterop option exists for a reason. And yes it's pervasive, but it also has to be because they fucked up/didn't consider that projects would use (and ship) CJS and ESM entry points. If they had, then you'd be able to use multiple export formats within the definitions, just as you can use multiple import formats alongside each other. They didn't think it through; or they just didn't want to bother.

Anyway, I've already stated this will be fixed in next major (just not as presented here). Until then, you can continue maintaining your fork, or ship your own klona definitions within your own type definitions. Or you can just build your library with rollup and you won't have any problems at all --- because it's an easily solved issue that TS just chooses not to deal with.

// rollup.config.js
import resolve from '@rollup/plugin-node-resolve';
import typescript from 'rollup-plugin-typescript2';

export default {
	input: 'src/default.ts',
	output: [{
		format: 'esm',
		file: 'lib/index.mjs',
	}, {
		format: 'cjs',
		file: 'lib/index.js',
		interop: false,
	}],
	external: [
		'klona',
	],
	plugins: [
		resolve(),
		typescript()
	]
}

// tsconfig.json
{
	"compilerOptions": {
		"rootDir": "src",
		"outDir": "lib",
		"noEmit": true,
		"module": "esnext",
		"target": "es5",
		"lib": ["esnext"],
		"sourceMap": true,
		"moduleResolution": "node",
		"declaration": true,
		"forceConsistentCasingInFileNames": true,
		"noImplicitReturns": true,
		"importHelpers": true,
		"strict": true,
		"strictFunctionTypes": false,
		"strictPropertyInitialization": false,
		"suppressImplicitAnyIndexErrors": true,
		"esModuleInterop": true,
		"noUnusedLocals": true,
		"experimentalDecorators": true,
		"downlevelIteration": true
	},
	"include": ["src"],
	"exclude": ["node_modules"]
}

// src/default.ts
import klona from 'klona';
console.log(klona);

// lib/index.js
'use strict';
var klona = require('klona');
console.log(klona);

// lib/index.mjs
import klona from 'klona';
console.log(klona);

[d-fischer]

If they had, then you'd be able to use multiple export formats within the definitions, just as you can use multiple import formats alongside each other.

That doesn't quite work this way. If you'd specify both export formats, that would mean that all given entry points export in all given ways. But actually, all given entry points only export in one of the ways, making the definitions invalid for both entry points, because they contain exports that don't actually exist in all possible runtime modules.

It's not a TS design issue. You either have to write one type definition and have your code adhere to it in all possible compilation results (which is not the case with the current state of this package), or you have to write one TS code base that compiles to different module formats.

I'm trying to do the latter, but as long as any of my dependencies don't do either, they can't work.

The only underlying design issue is an ES one. Default exports shouldn't exist. But now they do, and they're incompatible with the usual CommonJS way of importing, and people have to work around that incompatibility if they want to support both CJS and ESM. Rollup has one of these workarounds (it basically lets you import packages that are technically wrong!), but I don't use it because it impairs tree shaking. I'm trying to fix it the other way - building the package correctly from the ground up while still staying compatible with "legacy" importing (both from TS and not).

[hakimio]

@d-fischer can you create a package on npm with proper exports?

[lukeed]

Klona and dequal are both getting 2.0 this week, which includes the export change

[codecov-commenter]

Codecov Report

Merging #17 into next will decrease coverage by 100.00%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##              next     #17        +/-   ##
============================================
- Coverage   100.00%   0.00%   -100.00%     
============================================
  Files            1       1                
  Lines           38      38                
============================================
- Hits            38       0        -38     
- Misses           0      38        +38     

Impacted Files	Coverage Δ
src/index.js	0.00% <ø> (-100.00%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0298047...cd98d80. Read the comment docs.

[lukeed]

Hey @d-fischer, I'm trying to get this PR in to kick-off the 2.0 branch. However, I'm not allowed to push into your repo from my terminal (despite being able to edit files through GitHub UI?). Can you please rebase your branch from next? We're only keeping named export.

[d-fischer]

• master and next are even, rebasing is a noop. (This is probably why the CLI didn't want to do anything?)
• Why did you remove the TypeScript definition completely? I think the last commit should be reverted.

[hakimio]

@lukeed also, declare was there for a reason.

[lukeed]

@d-fischer Can you not make this difficult? I'm just trying to add your contribution. Your master (last I checked) is behind & GitHub is preventing me from rectifying anything on my end. The original types file is removed and changes should be applied to new index.d.ts file.

@hakimio I don't think so. export function klona<T>(input: T): T; from within type definitions works fine.

[d-fischer]

Rebasing doesn't work since the branch was merged into my own master already. I merged instead.

The TS definition removal still needs to be undone, probably.

[d-fischer]

Ah, the merge reintroduced the "old" definitions. That's definitely wrong.

[lukeed]

Thanks. You can add an extra commit that replaces the contents of your (new) index.d.ts file with:

export function klona<T>(input: T): T;

[lukeed]

Thank you 👍 Will now officially start the 2.0 work. Appreciate the callout & patience

[hakimio]

@lukeed

If a file has the extension .d.ts then each root level definition must have the declare keyword prefixed to it. This helps make it clear to the author that there will be no code emitted by TypeScript. The author needs to ensure that the declared item will exist at runtime.

Source

Should be:

export declare function klona<T>(input: T): T;

[yunyu]

This breaks in Typescript 4.7+'s "node16" resolution mode:

Could not find a declaration file for module 'klona/full'. '/Users/yunyu/server/node_modules/klona/full/index.mjs' implicitly has an 'any' type.
  Try `npm i --save-dev @types/klona` if it exists or add a new declaration (.d.ts) file containing `declare module 'klona/full';`ts(7016)

The version before this change works fine.

[d-fischer]

I'd suggest you create a new issue for that. It's not helpful to comment on a merge request that was merged more than two years ago, especially since your issue relates to a version of TypeScript that didn't even exist then (the current version at the date of merge was 3.9.7).