Closed mrdrogdrog closed 1 year ago
bump
The types are located in the root folder next to the original files. Hence, it shouldn't be necessary to explicitly link to the files..
Before you make another PR, can you please explain how your typescript version doesn't find the type definitions?
Sure.
The lib i'm building is a module. Therefore all imports have to be precise. When I import e.g. "array" the linker doesn't look for a "array.js". It looks for an export statement in the package.json that matches "array". It doesn't assume things anymore. Well and this is your package.json:
"./array.js": "./array.js",
"./dist/array.cjs": "./dist/array.cjs",
"./array": {
"import": "./array.js",
"require": "./dist/array.cjs"
},
If I try to import just "array" it uses the block at the bottom. But this block doesn't contain the type file reference. Therefore typescript can't find type definitions because the linker doesn't assume anything anymore.
Of course I could just import "array.js" instead of the alias export statement. The entry for this string is pointing directly to a .js file instead of a block which allows the linker to see the file and therefore look for a .d.ts
next to it... But then I can't import libs which just import "array" because then I'm gonna have the same problem. (to be precise: I have this problem with y-protocols which imports lib0/encoding
:upside_down_face: (another reason why I've created https://github.com/yjs/y-protocols/pull/21) ).
So I ask you to please merge this pull request. Otherwise I can't use this lib (and y-protocols) with esmodule libs and would need to replace it. :/
Did you have a chance to take a look at this again? Otherwise I'm gonna need to replace lib0 and y-protocols soon.
@mrdrogdrog
As I said before, the type declarations are located next to the original files. So "./dist/array.d.ts"` doesn't exist. This PR is untested and will break everyone else.
So before you make another PR. Please explain why the current approach doesn't work. What is your setup? Why does it work for everyone else but you?
Second, you need to change your PR to point to the actual type declarations: ./array.d.ts
not ./dist/array.d.ts
Third, you need to test the PR and ensure it actually works. Please also show me how I can verify that it works in the future.
As I said before, the type declarations are located next to the original files. So "./dist/array.d.ts"` doesn't exist.
Nope. they exist.
But okay. Since you prefer the top level type files I changed the code.
This PR is untested and will break everyone else.
No. I've tested it locally and it works. It doesn't break anything since the export object is only used if typescript is using the ESM resolver.
So before you make another PR. Please explain why the current approach doesn't work. What is your setup? Why does it work for everyone else but you?
Like I explained before here a full module setup relies only on the export object and doesn't assume the location of type files.
I guess most of the people use the default configuration of typescript for module resolution which is the commonjs module resolver which still assumes the location of type files. My project(s) (at least the libraries) use rollup to create MJS and CJS exports. I create these libs in typescript and have configured typescript to produce ESM files. Then I use rollup (microbundle to be precise) to generate cjs code. I do this by telling typescript to use the newer module resolver which relies on the exports object. Therefore the dependencies have to be either commonjs packages or export the type declaration files.
I recorded this quick video to demonstrate the effect of the modified package.json . I used my lib's test cases because the tests are also written in typescript and therefore also use the ESM resolver.
https://user-images.githubusercontent.com/6124140/205284159-6e238004-abd4-45ef-abe5-8f0a42714f18.mp4
btw: yjs exports the types correctly https://github.com/yjs/yjs/blob/main/package.json#L29 . That's why this lib doesn't produce such errors.
Same as https://github.com/yjs/y-protocols/pull/21