arethetypeswrong / arethetypeswrong.github.io

Tool for analyzing TypeScript types of npm packages
https://arethetypeswrong.github.io
MIT License
1.12k stars 38 forks source link

How to make this work with svelte? #171

Closed RobbieTheWagner closed 3 months ago

RobbieTheWagner commented 3 months ago

I am having a really terrible time trying to get svelte imports to not break this package. Has anyone had success with getting this to run with importing svelte components? I get Internal resolution error for all of them.

RobbieTheWagner commented 3 months ago

I am currently hopelessly stuck https://github.com/shepherd-pro/shepherd/actions/runs/9164124106/job/25194730222?pr=2820

RobbieTheWagner commented 3 months ago

I was able to get Svelte to generate some declaration files and layer on a ton of hacks to get some of my issues fixed, however I am still plagued by Internal resolution error. Is there any way to get more info about where the offending code might be? It's rather frustrating that it does not tell you where to look.

andrewbranch commented 3 months ago

If you’re using the CLI, you can get a dump of all the info it sees by using --json. Having this tool understand .svelte files is out of scope. I would recommend ignoring Internal Resolution Error problems and any entrypoints that point directly to .svelte files.

RobbieTheWagner commented 3 months ago

@andrewbranch thank you for the reply! So just to clarify, there is no way to ignore the .svelte files imported? Having declaration files for them doesn't seem to help either. I did confirm with the json output that this is the issue:

        "kind": "InternalResolutionError",
        "resolutionOption": "node16",
        "fileName": "/node_modules/shepherd.js/dist/esm/tour.d.ts",
        "moduleSpecifier": "./components/shepherd-modal.svelte",
RobbieTheWagner commented 3 months ago

I do see further down this message "File '/node_modules/shepherd.js/dist/esm/components/shepherd-modal.d.svelte.ts' does not exist." implying if we had a .d.svelte.ts file, it would be happy? Svelte declaration files are .svelte.d.ts though. If we could make this tool look for the correct extension, maybe it would work?

andrewbranch commented 3 months ago

implying if we had a .d.svelte.ts file, it would be happy?

Yep!

Svelte declaration files are .svelte.d.ts though. If we could make this tool look for the correct extension, maybe it would work?

.d.svelte.ts is the correct declaration file extension for .svelte. This tool doesn’t have its own resolution logic or opinions, it’s just showing what TypeScript is looking for. If Svelte is generating .svelte.d.ts files, it’s doing the wrong thing, either on purpose to support TypeScript versions older than 5.0, or because they didn’t know how to support typings for arbitrary extensions after 5.0.

That said, I’m unable to comment on how valuable it is to get this working for a library of Svelte components. Svelte is not valid TypeScript or JavaScript, so when users are using Svelte, they’re using a different/wrapped compiler and language service, which is totally out of scope for this tool to reason about, so I’m not sure how much any of the answers from this tool will make sense.

RobbieTheWagner commented 3 months ago

@andrewbranch I am not concerned about the svelte types actually being used, I just need the import of a svelte file to be okay with this tool. I can try renaming all the svelte declaration files and see what happens, but this is output straight from svelte2tsx which seems to be the official way to generate declarations for svelte.

RobbieTheWagner commented 3 months ago

@andrewbranch renaming the svelte declaration files gets ESM working, but CJS still gives me Internal resolution error, so I am not sure what to do there. I know Svelte is ESM only these days, so I imagine svelte2tsx is just generating ESM declaration files, but I am not sure how to update things to make it work with CJS.

RobbieTheWagner commented 3 months ago

Okay, I was wrong, the svelte declaration files are fine. The problem lies in the fact that we use .cjs for the CJS output, and attw wants there to be a .d.cts file for them, however our imports do not use those paths in the real files, so things break.

RobbieTheWagner commented 3 months ago

Would we be able to allow .d.cts as a possible path to check in attw?

RobbieTheWagner commented 3 months ago

My current workaround is to ship both .d.ts and .d.cts which I think is probably wrong. I am not sure that attw should be enforcing that we have to use .cjs for CJS bundles. If I don't, it doesn't find the types at all, but when I do it then cannot follow the normal .ts imports in the files.

I suppose I could make all of our files .mts instead of using type: module and then I could use normal .js for the CJS bundle maybe? This all feels pretty murky to me, and I am not sure what I should be doing.

timostamm commented 3 months ago

Hey Robbie, if all of this feels murky to you, welcome to the club. The situation with CJS/ESM, and types is a mess, and it is impressively difficult to publish a package correctly. You chose to add a custom compiler into the mix to make it more challenging 😄

If you want to package a Svelte component, follow the Svelte documentation on the topic. It will force you to at least add two exports:

  "types": "./dist/index.d.ts",
  "svelte": "./dist/index.js"

The result will work for a consumer using the Svelte compiler, but it's unlikely to work for much else. There's no export for Node10 module resolution, no export for CJS, and no export for ESM, only for the "svelte" condition.

This is probably why you see errors reported by attw - your types are wrong.

timostamm commented 3 months ago

If you only want to package a Svelte component, that's okay. Your components will only be consumed by the Svelte compiler.

If you are not simply packaging a Svelte component, and do want your package to be available to as many consumers as possible, you'll have to fix issues reported by attw.

That is not trivial, and it's possible that @sveltejs/package or svelte2tsx do not properly support it. If your really want to go for this, read up on "module resolution" and "dual packages". You may have to file issues with Svelte.

RobbieTheWagner commented 3 months ago

@timostamm the issues are with attw, IMO, if you read above svelte2tsx generates .svelte.d.ts files, but this tool requires .d.svelte.ts files, which is fine, I manually renamed them, but annoying.

The big issue here is you can easily ship two bundles with .js extensions, one in CJS and another in ESM and that should be valid, but this tool gets very upset and forces you to ship .cjs files, which in turn breaks imports because we just use .ts for all our files.

andrewbranch commented 3 months ago

It’s not this tool that requires .d.svelte.ts files, it’s TypeScript. This tool is simply showing what works and doesn’t work for users in different configurations of tsc.

The big issue here is you can easily ship two bundles with .js extensions, one in CJS and another in ESM and that should be valid

Valid for whom? This tool is showing you that such a configuration is broken in Node.js. This tool has no opinions of its own. If you want to ship something that only works in the Svelte universe, go for it! It doesn’t seem like this tool is going to be relevant for that use case.

RobbieTheWagner commented 3 months ago

@andrewbranch

Valid for whom?

For everyone. Perhaps I misunderstand how exports work, but if you ship a dist/esm and a dist/cjs but just use .js extensions for all your files, you should be able to tell exports like "import": "./dist/esm/foo.js" or "require": "./dist/cjs/foo.js" which should tell things using ESM or CJS which files to use, thus not needing to manually make them .cjs or .mjs extensions. Is that not correct?

timostamm commented 3 months ago

@RobbieTheWagner, Node.js can not tell what type your export is, even if it's behind an import or export condition. You have to file an issue with Node.js if you want this behavior changed. attw cannot do anything about the behavior of Node.js except warn you that your exports are broken for Node.js module resolution.