unist-util-map types Node and MapFunction stopped to be exported between v3 and v3.1.1

[revelt]

Initial checklist

• I read the support docs
• I read the contributing guide
• I agree to follow the code of conduct
• I searched issues and couldn’t find anything (or linked relevant results below)

Affected packages and versions

unist-util-map 3.1.1

Link to runnable example

https://stackblitz.com/edit/typescript-gjwbvb?file=index.ts

Steps to reproduce

Start a new sandbox and install the latest unist-util-map v3.1.1:
https://stackblitz.com/edit/typescript-gjwbvb?file=index.ts
notice the MapFunction and Node (and Parent) types are not exported any more

but they used to be exported on v3:
https://stackblitz.com/edit/typescript-qh1hrn?file=index.ts

which means, we introduced a breaking semver change during a minor bump!

Now, other consumers such as remark-directive-rehype are importing the unist-util-map with a caret semver, which causes them to break on fresh installs (stale lockfiles still contain working v3 references, so everything seems fine for those who have stale lockfiles and their package manager uses exact stale version v3)
https://github.com/IGassmann/remark-directive-rehype/blob/5dec9b98dcf8e071d2bd5b776f96123f459492f9/package.json#L34

Expected behavior

Let's restore the old type exports, specifically:

export type Node = import('unist').Node
export type Parent = import('unist').Parent
export type MapFunction = (node: Node, index?: number, parent?: Parent) => Node

If we really want to remove the types, then issue a patch with types restored and bump the major version, publishing this program with types removed.

Generally, to prevent mishaps like this, it's best to migrate the source to .ts source files and let the TypeScript generate the types automatically, using a bundler. The JSDoc can be used for examples etc but not as a source of type definitions. That's the setup I use myself, I use esbuild to bundle ESM (and sometimes optionally IIFE too) and rollup with rollup-plugin-dts to bundle up the type definition files. The JSDoc-based types from .js sources are a liability.

Actual behavior

type exports are missing currently

Affected runtime and version

n/a

Affected package manager and version

tested on both yarn and npm

Affected OS and version

n/a

Build and bundle tools

Other (please specify in steps to reproduce)

[JounQin]

If we really want to remove the types, then issue a patch with types restored and bump the major version, publishing this program with types removed.

While I agree the types are removed unexpectedly, but I'm a bit disagree we should consider types changes as major.

---

I raised #7 to fix your usage.

[revelt]

hi Joun! Thanks for the quick turnaround!

[ChristianMurphy]

This feels a bit like an instance of Hyrum's law.
Those interfaces were incidentally exported and were never part of this library's API.
I'm caution creating a semver major over a single plugin relying on an internal implementation detail.

IMO, this should be fixed in remark-directive-rehype by importing the types from the correct upstream location.

Generally, to prevent mishaps like this, it's best to migrate the source to .ts source files and let the TypeScript generate the types automatically, using a bundler

Unified uses plain JavaScript to avoid the issues needing layers of compilers/transpilers/bundlers.
If https://github.com/tc39/proposal-type-annotations becomes standard, we may move to that syntax. Until then JSDocs work well enough.

[wooorm]

bit disagree we should consider types changes as major.
— Joun

I particularly agree about Node and Parent.
Those were accidentally exported (because TS treats JSDoc imports as exports too).
They were never documented, never supposed to be used, and that mistake was fixed.

To prevent that happening in the future, we now document what is part of the API: https://github.com/syntax-tree/unist-util-map#types.

[JounQin]

To prevent that happening in the future, we now document what is part of the API

So missing MapFunctions from types is indeed unexpectedly while omitting Parent is intended, right?

I may need to remove https://github.com/syntax-tree/unist-util-map/pull/7/files#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346R3 in this case.

[remcohaszing]

(because TS treats JSDoc imports as exports too)

TS treats JSDoc typedefs as exports, not imports. To assign every import to a typedef is a stylistic choice that leads to issues like this.

Many of these issues would be resolved by microsoft/TypeScript#46407.

Regardless, I agree people shouldn’t import these type aliases from packages like unist-util-map. They should just import from (@types/)unist. So I don’t really consider their removal to be a breaking change.

[wooorm]

To assign every import to a typedef is a stylistic choice that leads to issues like this.
— Remco

It is a choice, though I don’t think it’s just stylistic to import things from other files at the top of a file: that’s how a lot of languages work, and it works well.
I think that it is highly impractical to use a different “stylistic” choice. To only use “import expressions” and never assign them. It makes the file verbose because long lines are repeated a lot, and hard to maintain because it is unclear which dependencies are in use.

---

I am assuming that the @revelt does not care about the addition of MapFunction to index.js, if Node and Parent will not be, as a type error in remark-directive-rehype will keep ocurring.

I am personally open to expose MapFunction from index.js, if this indeed properly works.
I was under the impression that generic parameters could not be exported like this? If it is supported, then I don’t think it was earlier? We should perhaps check that it actually works, that the generic parameter can be set?
If all of this works, we can update the documentation, that MapFunction should be imported from this package, and not from complex-types.

[JounQin]

I was under the impression that generic parameters could not be exported like this?

It is supported, you can review my PR #7

[wooorm]

Then I’d still wonder about the point that came after that: “If it is supported, then I don’t think it was earlier”, as in, I wonder which TS version started supporting this!

[JounQin]

I don't know. 😂

I just searched and found the solution.

[wooorm]

OK, I checked, it works in TS ^4.5, it does not work in 4.4 and lower. The root problem then is:

index.js:6:21 - error TS1069: Unexpected token. A type parameter name was expected without curly braces.

6  * @template {Node} [Tree=Node]

The last release of TS 4.4 was on 2021-12-10. Do we think it’s fine to break that? (I am open to it)

[JounQin]

Do we think it’s fine to break that? (I am open to it)

I'm fine with it, and it should only affect contributors of this package, right?

[wooorm]

Released in 3.1.2! Thanks @JounQin!