fix: add types location for cjs/src/

[rvagg]

Now we're publishing a "main", TypeScript is using that rather than the
"exports" map. It points to the CJS published version, located in cjs/src
in the bundle. This adds a descriptor that tells TypeScript to look for types
for that location in the types/, which should be the same as to what it finds
in the export map.

Ref: #64 (comment)

[rvagg]

OK, some further diagnosis of the problem this is fixing:

These fail:

/* @typedef {import('multiformats').CID} CID */
import { CID } from 'multiformats'

But these don't:

import CID from 'multiformats/cid
import raw from 'multiformats/codecs/raw

So I'd assume from that that TypeScript is reading 'multiformats' through the "main" lens, but the rest of the imports through the "exports" lens. It may be enough to just "typesVersions" "cjs/src/index.js", the file "main" points to.

I don't know the impact on consuming these things via CJS, we may be doing that in the js-ipfs stack so it might be worth figuring out.

[Gozala]

As far as I know:

1. You can resolve ‘multiformats’ case simply by adding “types” field to package.json that would correspond to a ‘.d.ts’ file for “main”.
2. I would be really surprised if proposed patch solves a problem (for dependent libs). As far as I know TS does not recognize exports field at all, only reason it worked is because “typeVersions” mapped all modules to types dir which is where .d.ts files were emitted. So “multiformats/cid” would resolve to “multiformats/types/cid.d.ts”.

I think best way to figure out what resolution does is to run tsc —traceResolution

[vmx]

I've tested this PR with ipfs/js-ipfs-unixfs#116, which is using CommonJS modules and it works for me. I don't see TypeScript failures.

[rvagg]

Thanks for the explanation @Gozala. I think it might be best to isolate this to just the "main", as a documentation tactic mainly (i.e. to make people question why it's so specific and hopefully they'll realise why it's there -- since we can't include docs nicely in package.json).

Thanks for testing @vmx! Great news. I'll push this forward tomorrow.

[achingbrain]

FWIW this will error with Import declaration conflicts with local declaration of 'CID'.ts(2440) so I don't think it's something we should try to fix:

/* @typedef {import('multiformats').CID} CID */
import { CID } from 'multiformats'

[Gozala]

@achingbrain mentioned having issues with this today, which I think would be addressed by adding types field to package.json.

[Gozala]

I am not sure I understand what this change does. Do we ever import path like multiformats/cjs/src/... because that is what I think it would catch here.

If this is about fixing

import { CID } from 'multiformats'

I think suggestion above is probably adding following to package.json is a better way to go:

{
  "types": "./types/index.d.ts"
}

[rvagg]

comment below #66 (comment)

this is for when we publish a "main" pointing to cjs/src/index.js, that mapping causes problems for consumers of this package

[Gozala]

Looking at this PR I'm realizing that source typedef maps generated are probably going to be broken. Because generated types def maps will point to files in ../src from ./dist, which will not exist because I think we just publish ./dist ?

Seems like ipjs should use a better strategy to do publishing otherwise paths mapping going to be a pain.

[rvagg]

hah, yeah ... https://unpkg.com/browse/[email protected]/types/index.d.ts.map

What are the implications of this for users @Gozala? I don't know how these .ts.map files are consumed.

[Gozala]

What are the implications of this for users @Gozala? I don't know how these .ts.map files are consumed.

When you click on hints in vscode, it won't be able to take you to definition in the actual source file. I'm guessing it would take you to either d.ts file instead or nowhere, because .ts.map files were introduced specifically to jump to the source coed as opposed to generated defs.

[vmx]

This is a reply to comment #66 (comment)

import CID from 'multiformats/cid
import raw from 'multiformats/codecs/raw

In commonjs modules, these kind of imports don't work (same with and without this PR). When I do

const CID = require('multiformats/cid')

I get

error TS2339: Property 'parse' does not exist on type 'typeof import("/some/path/js-multiformats/dist/types/cid")'

I also tried adding

/**
 * @typedef {import('multiformats/cid')} CID
 */

but that didn't help either.

[Gozala]

This is a reply to comment #66 (comment)

import CID from 'multiformats/cid
import raw from 'multiformats/codecs/raw

In commonjs modules, these kind of imports don't work (same with and without this PR). When I do

const CID = require('multiformats/cid')

I get

error TS2339: Property 'parse' does not exist on type 'typeof import("/some/path/js-multiformats/dist/types/cid")'

I also tried adding

/**
 * @typedef {import('multiformats/cid')} CID
 */

but that didn't help either.

I think the problem is that from TS perspective CID is under the default name meaning should be imported as require('multiformats/cid').default while ipjs instead generates cjs output that puts CID into module.exports.

From my reading of esModuleInterop option it seems like it would change TS to do what ipfs does, but I'm not sure if it is true as I remember having this kind of issues in js-ipfs despite the option set (although it might have been reverse problem there).

Overall I think we would make our lives easier if we just used named exports as opposed to default exports as various tools seems to make incompatible decisions about how to interpret default exports resulting in this kind of problems.

[vmx]

Overall I think we would make our lives easier if we just used named exports as opposed to default exports as various tools seems to make incompatible decisions about how to interpret default exports resulting in this kind of problems.

After having a quick look (I don't really consider myself having a good understanding of this whole CJS/ESM/TS issues), I agree named exports might be the cleanest/easiest solution. I also found https://humanwhocodes.com/blog/2019/01/stop-using-default-exports-javascript-module/ which I found quite convincing.

[rvagg]

I've included a version of this in #70, and I'll describe it there. Using "types" doesn't work, the point here is that publishing with a "main" that points to cjs/src/index.js seems to need to have a mapping to the types file for that. I don't know why this is required but it is for plain imports of 'multiformats' when consumed. If you want to see it at work, checkout ipld/js-car#7 and link the built dist directory here to the multiformats in node_modules there and try and run build:types there to see the errors ("no types" whenever there are imports of 'multiformats').

[Gozala]

I've included a version of this in #70, and I'll describe it there. Using "types" doesn't work

😮 I do not understand. So here is the link to module resolution strategy that TS uses https://www.typescriptlang.org/docs/handbook/module-resolution.html#how-typescript-resolves-modules quoting relevant bits below:

Similarly, a non-relative import will follow the Node.js resolution logic, first looking up a file, then looking up an applicable folder. So import { b } from "moduleB" in source file /root/src/moduleA.ts would result in the following lookups:

/root/src/node_modules/moduleB.ts
/root/src/node_modules/moduleB.tsx
/root/src/node_modules/moduleB.d.ts
/root/src/node_modules/moduleB/package.json (if it specifies a "types" property)
/root/src/node_modules/@types/moduleB.d.ts
/root/src/node_modules/moduleB/index.ts
/root/src/node_modules/moduleB/index.tsx
/root/src/node_modules/moduleB/index.d.ts

/root/node_modules/moduleB.ts
/root/node_modules/moduleB.tsx
/root/node_modules/moduleB.d.ts
/root/node_modules/moduleB/package.json (if it specifies a "types" property)
/root/node_modules/@types/moduleB.d.ts
/root/node_modules/moduleB/index.ts
/root/node_modules/moduleB/index.tsx
/root/node_modules/moduleB/index.d.ts

/node_modules/moduleB.ts
/node_modules/moduleB.tsx
/node_modules/moduleB.d.ts
/node_modules/moduleB/package.json (if it specifies a "types" property)
/node_modules/@types/moduleB.d.ts
/node_modules/moduleB/index.ts
/node_modules/moduleB/index.tsx
/node_modules/moduleB/index.d.ts

Don’t be intimidated by the number of steps here - TypeScript is still only jumping up directories twice at steps (9) and (17). This is really no more complex than what Node.js itself is doing.

[Gozala]

I'll post updates in the ipld/js-car#7

[Gozala]

I've included a version of this in #70, and I'll describe it there. Using "types" doesn't work, the point here is that publishing with a "main" that points to cjs/src/index.js seems to need to have a mapping to the types file for that. I don't know why this is required but it is for plain imports of 'multiformats' when consumed. If you want to see it at work, checkout ipld/js-car#7 and link the built dist directory here to the multiformats in node_modules there and try and run build:types there to see the errors ("no types" whenever there are imports of 'multiformats').

Figured out what the problem was and submitted #72 to address it. TL;DR TS bug microsoft/TypeScript#41284 causes double path substitution when "types" field is present resulting in a wrong path.