Fix incorrect type-declaration

[manuth]

The current type-declarations of markdown-it-anchor are incorrect.
Thus, this module can only be consumed if esModuleInterop is set to true in the project which tries to consume markdown-it-anchor.

This PR changes the type-declarations properly.

All, markdown-it, markdown-it/lib/token and markdown-it/lib/rules_core/state_core use a module.exports = [...]-statement rather than a export default [,,,]-statement.

However, as seen here, the imports are written as if a export default [...]-module is being imported:

markdown-it-anchor/types/index.d.ts

Lines 1 to 3 in 5592824

	import MarkdownIt from 'markdown-it';
	import Token from 'markdown-it/lib/token';
	import State from 'markdown-it/lib/rules_core/state_core';

Therefore, these import-statements need to be changed to import x = require("module-x");.

What's more, the markdown-it-anchor-module uses an export default x-statement rather than a module.exports = x-statement.
However, the type-declaration suggests that the module is a module.exports = x-module:

markdown-it-anchor/types/index.d.ts

Line 65 in 5592824

export = anchor;

In order to fix this, I changed this statement to export default anchor;.

Also I added @types/markdown-it to the dependencies as the type-declarations depend on it.
Optionally you could add it to peerDependencies but I'm not quite sure whether that's the right way to got, tbh.

I'd love to see this implemented in a new patch-version as soon as possible.
Please note, that this change was taken from the previous type-declaration available on DefinitelyTyped:
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/9c1e78c99106fc3e1be574ca1b968d01963e032d/types/markdown-it-anchor/index.d.ts

[manuth]

For everyone facing issues with the type-declarations:
Install @types/markdown-it-anchor@9.12.4 and add the following to your tsconfig.jsons compilerOptions:

{
    "paths": {
        "markdown-it-anchor": [
            "./node_modules/@types/markdown-it-anchor"
        ]
    }
}

This will make typescript use the type-declarations found at DefinitelyTyped.

[manuth]

Awesome! Thanks a ton for taking care

[valeriangalliat]

Thank you so much for your PR!

I don't know TypeScript very well, before I publish it, can you let me know if changing export = anchor; to export default anchor; is considered a breaking change in the TypeScript world? So I know what kind of version to update. I'm worried if I make this a patch or "feature" it might break other people's code.

Also I'm wondering, would it make sense to move @types/markdown-it to the devDependencies? I don't believe that types are required at runtime.

Cheers!

[valeriangalliat]

Edit: ok it seems when using microbundle to derive both CommonJS and UMD dist files, they use export default anchor; in the unique .d.ts file so that matches this PR, I'll assume that means it's indeed the right way to handle this!

Old comment

Also now I'm thinking about it, the actual JS code does module.exports = markdownItAnchor, isn't that supposed to be reflected by TypeScript by export = anchor as it was initially the case?

More specifically this module uses the following pattern in package.json to support both CommonJS and UMD:

  "main": "dist/markdownItAnchor.js",
  "module": "dist/markdownItAnchor.mjs",

I'm not sure what would be the proper way with TypeScript to handle that, I believe both import anchor from 'markdown-it-anchor and const anchor = require('markdown-it-anchor') should work and I'm worried if I publish types to reflect the UMD style people using CommonJS style with TypeScript (which should work?) might also complain? What do you think?

[manuth]

Sadly, I don't know what the best practice is.
In this case, @types/markdown-it either must be present in dependencies or peerDependencies.

I'll try to make up some simple examples.

Let's say I create a package which prints error-messages using markdown-it like this:

import Markdown = require("markdown-it");

try {
    doWork();
} catch (error) {
    return new MarkdownIt.render(
        dedent(`
            # An Error Occurred
           ~~~
            ${error}
            ~~~`));
}

In this case, no types from @types/markdown-it are exposed by this package. Therefore in this example, the right place for @types/markdown-it are the devDependencies.

But as soon as you expose types from your @types/*-dependencies, @types/* must be in your dependencies or peerDependencies.

A few examples:

import MarkdownIt = require("markdown-it");

/* Exposing types by extending a class */
export class AwesomeMarkdownIt extends MarkdownIt { }

/* Exposing types by accepting parameters */
export class Converter {
    public Convert(parser: MarkdownIt, md: string) {
        return parser.render(string);
    }
}

/* Exposing types through properties and fields */
export class Converter {
    public get Parser(): MarkdownIt {
        return null;
    }
}

// or

export class Converter {
    public Parser: MarkdownIt = null;
}

In these cases, types from @types/markdown-it are exposed and thus your package depends on @types/markdown-it.
I think, dependencies is the best place to have @types/markdown-it as I don't believe you can have multiple @types/markdown-it packages w/ different version in your peerDependencies (?).

[valeriangalliat]

Ah that makes sense, while the runtime doesn't need the types, putting them in devDependencies would mean they're only installed when developing on markdown-it-anchor itself and not for the development of projects that depend on it. Sad that we don't have a level of granularity to distinguish between "needed at runtime" and "needed during development in parent packages".

I'll move @types/markdown-it to peerDependencies since that's where we have the markdown-it dependency itself, and I'll make it "@types/markdown-it": "*" as we also allow any version of the markdown-it package itself, I think that'll yield the most consistent results. Let me know if you think this will cause problems :)

[manuth]

Edit: ok it seems when using microbundle to derive both CommonJS and UMD dist files, they use export default anchor; in the unique .d.ts file so that matches this PR, I'll assume that means it's indeed the right way to handle this!

Old comment
Also now I'm thinking about it, the actual JS code does module.exports = markdownItAnchor, isn't that supposed to be reflected by TypeScript by export = anchor as it was initially the case?

More specifically this module uses the following pattern in package.json to support both CommonJS and UMD:

  "main": "dist/markdownItAnchor.js",
  "module": "dist/markdownItAnchor.mjs",

I'm not sure what would be the proper way with TypeScript to handle that, I believe both import anchor from 'markdown-it-anchor and const anchor = require('markdown-it-anchor') should work and I'm worried if I publish types to reflect the UMD style people using CommonJS style with TypeScript (which should work?) might also complain? What do you think?

I just had a look at an example esm/mjs module here: https://github.com/gfmio/typescript-esm-cjs-hybrid-example

Looks like you have to use exports.default = x in your cjs, too if you want to create a hybrid module that is compatible with TypeScript.

I cloned said repository and changed the ./hybrid/src/hybrid.ts to include a export =-statement.

Trying to compile this project by running npm run build inside the ./hybrid-directory results with following error:

src/hybrid.ts:6:2 - error TS1203: Export assignment cannot be used when targeting ECMAScript modules. Consider using 'export default' or another module format instead.

  6  export = function sayHello(): boolean {
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  7     console.log("Hello, World!");
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
...
 11     return typeof exports === "undefined";
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 12 }
    ~

[manuth]

Ah that makes sense, while the runtime doesn't need the types, putting them in devDependencies would mean they're only installed when developing on markdown-it-anchor itself and not for the development of projects that depend on it. Sad that we don't have a level of granularity to distinguish between "needed at runtime" and "needed during development in parent packages".

I'll move @types/markdown-it to peerDependencies since that's where we have the markdown-it dependency itself, and I'll make it "@types/markdown-it": "*" as we also allow any version of the markdown-it package itself, I think that'll yield the most consistent results. Let me know if you think this will cause problems :)

Sounds great, thanks for your effort 😄

[manuth]

I found a solution here: developit/microbundle#712
I'll go and open up another PR concerning this!

Thanks a lot for being patient

[valeriangalliat]

Oh interesting. I also found that I could get TypeScript to accept both syntaxes in a way that works at runtime by doing anchor.default = anchor in index.js... kinda hacky but simple enough that I'm fine with it. I think that's kinda the inverse way of the issue you linked.

I believe that developit/microbundle#712 would be more if markdown-it-anchor used named exports, but currently since we only have a default export that's why we can do const anchor = require('markdown-it-anchor') which is nice (and it seems that's what they recommend doing to fix the inverse issue).

I added the anchor.default = anchor in 6fcc502 feel free to npm install valeriangalliat/markdown-it-anchor to try it from the current master but I believe that fixes the problem in a backwards compatible way! Works on my side using either syntaxes.

[manuth]

Yeah, that works great 😄
Thanks a lot!

You could even go as far and add a separate index.cjs.js-file where you add the default-workaround (just like in the linked PR) to have the workaround only present in cjs and umd-files.

[valeriangalliat]

Cool, I just published 8.3.1 with that patch! I'll leave the .default hack everywhere as I would have to further hack around microbundle itself to only apply it in the .cjs file and it's probably not worth the hassle :P

[manuth]

Agreed!
Thanks a bunch for your effort 😄