Closed manuth closed 2 years ago
For everyone facing issues with the type-declarations:
Install @types/markdown-it-anchor@9.12.4
and add the following to your tsconfig.json
s compilerOptions
:
{
"paths": {
"markdown-it-anchor": [
"./node_modules/@types/markdown-it-anchor"
]
}
}
This will make typescript use the type-declarations found at DefinitelyTyped
.
Awesome! Thanks a ton for taking care
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!
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!
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
(?).
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 :)
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 byexport = 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
andconst 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 }
~
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
topeerDependencies
since that's where we have themarkdown-it
dependency itself, and I'll make it"@types/markdown-it": "*"
as we also allow any version of themarkdown-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 😄
I found a solution here: https://github.com/developit/microbundle/issues/712 I'll go and open up another PR concerning this!
Thanks a lot for being patient
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 https://github.com/valeriangalliat/markdown-it-anchor/commit/6fcc50233d593458aa883e5b515cb8311114555c 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.
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.
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
Agreed! Thanks a bunch for your effort 😄
The current type-declarations of
markdown-it-anchor
are incorrect. Thus, this module can only be consumed ifesModuleInterop
is set totrue
in the project which tries to consumemarkdown-it-anchor
.This PR changes the type-declarations properly.
All,
markdown-it
,markdown-it/lib/token
andmarkdown-it/lib/rules_core/state_core
use amodule.exports = [...]
-statement rather than aexport default [,,,]
-statement.However, as seen here, the imports are written as if a
export default [...]
-module is being imported: https://github.com/valeriangalliat/markdown-it-anchor/blob/55928244bb98e77fd98885d3840cb16c3ed6c085/types/index.d.ts#L1-L3Therefore, these import-statements need to be changed to
import x = require("module-x");
.What's more, the
markdown-it-anchor
-module uses anexport default x
-statement rather than amodule.exports = x
-statement. However, the type-declaration suggests that the module is amodule.exports = x
-module: https://github.com/valeriangalliat/markdown-it-anchor/blob/55928244bb98e77fd98885d3840cb16c3ed6c085/types/index.d.ts#L65In order to fix this, I changed this statement to
export default anchor;
.Also I added
@types/markdown-it
to thedependencies
as the type-declarations depend on it. Optionally you could add it topeerDependencies
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