xstate-tools icon indicating copy to clipboard operation
xstate-tools copied to clipboard

Support for mts/cts file extensions

Open jscheid opened this issue 2 years ago • 5 comments

I'm using Typescript's experimental support for ESM, using the .mts extension. This isn't currently recognized by xstate-cli. Could the requirements for the extension be relaxed?

Also, when a file extension isn't recognized, the (source) file gets deleted because in that case pathToSave is identical to opts.filePath.

jscheid avatar Mar 17 '22 10:03 jscheid

Also, as described on that page, Typescript with ESM requires imports to have explicit extensions, as in tsTypes: {} as import("./machine.typegen.js").Typegen0. I can understand if you're not interested in adding support for it at this stage. It's easy to work around, the issue with the extension slightly less so.

jscheid avatar Mar 17 '22 10:03 jscheid

I'm using Typescript's experimental support for ESM, using the .mts extension. This isn't currently recognized by xstate-cli. Could the requirements for the extension be relaxed?

I've prepared a fix for this here: https://github.com/statelyai/xstate-tools/pull/109

Also, when a file extension isn't recognized, the (source) file gets deleted because in that case pathToSave is identical to opts.filePath.

That only happens when the file doesn't have any machines in it, right? I believe that I have also fixed that in my PR because now those 2 paths should always be different.

Also, as described on that page, Typescript with ESM requires imports to have explicit extensions, as in tsTypes: {} as import("./machine.typegen.js").Typegen0. I can understand if you're not interested in adding support for it at this stage. It's easy to work around, the issue with the extension slightly less so.

How do you plan to work around this? 🤔 It would be great to support this but IIRC TS actually complains about an explicit extension when this experimental setting is not enabled. So this would have to be done conditionally. And if I understand correctly, this isn't specific to .mts and .cts extensions but rather to the tsconfig.json setting, right? So to handle this correctly we'd have to locate that and check the used option.

Andarist avatar Mar 17 '22 11:03 Andarist

Thanks for the quick fix, look good!

That only happens when the file doesn't have any machines in it, right? I believe that I have also fixed that in my PR because now those 2 paths should always be different.

I think it happened regardless of what's in the file, because it doesn't even get to reading it. But yes, with your PR I can't see it happening anymore.

How do you plan to work around this? 🤔 It would be great to support this but IIRC TS actually complains about an explicit extension when this experimental setting is not enabled. So this would have to be done conditionally. And if I understand correctly, this isn't specific to .mts and .cts extensions but rather to the tsconfig.json setting, right? So to handle this correctly we'd have to locate that and check the used option.

Hmm, yes that's tricky. I guess another option is to add a new command line option, so that the user can enable the .js extension explicitly - or maybe even control the full suffix (.typegen)? I'd be perfectly happy with that.

jscheid avatar Mar 17 '22 13:03 jscheid

Hmm, yes that's tricky. I guess another option is to add a new command line option, so that the user can enable the .js extension explicitly - or maybe even control the full suffix (.typegen)? I'd be perfectly happy with that.

Ideally, I think that such things should just "work out of the box" to deliver the best DX. There is little reason to make this actually configurable and that would only raise complexity for our consumers.

At the VSCode side of things I think that we can query the correct tsconfig info by registering a plugin into the language server, some docs can be found here. This sounds like a fun hack and I hope to get to this in the coming weeks.

In the CLI we can just try to read the nearest tsconfig.json to the file. Of course, this strategy could also be used by the VScode plugin - but I'm somewhat more inclined to reuse the 100% battle-tested algorithm used by the TS server.

Andarist avatar Mar 18 '22 10:03 Andarist

I was thinking along the lines of "perfect is the enemy of good" but if you think you'll get around to it at some point, I'm all for it! Thanks again.

jscheid avatar Mar 18 '22 10:03 jscheid

In the CLI we can just try to read the nearest tsconfig.json

You would not only have to read the tsconfig.json but also the type in package.json.

tsconfig (module) package.json (type) .typegen .typegen.js
CommonJS commonjs :white_check_mark: :white_check_mark:
CommonJS module :x: :white_check_mark:
NodeNext commonjs :white_check_mark: :white_check_mark:
NodeNext module :x: :white_check_mark:
ES2022 commonjs :white_check_mark: :white_check_mark:
ES2022 module :x: :white_check_mark:

pixtron avatar Mar 23 '23 08:03 pixtron

Yes, and that should read the nearest package.json and not the root one. I would really like to just "offload" this to TS API though - reimplementing all of this this is a chore :s

However, I also wonder if we could just move the generated types into a declaration file - maybe this way we could just refer to it with an explicit extension at all times and it could "just work" in all contexts. Gonna try to ask the TS team about it.

Andarist avatar Mar 23 '23 10:03 Andarist

+1 for this!

Is there a recommended solution for using js extension imports with typegen? For me its completely broken where removing the extension from the import renders the typegen useless. Adding the extension fixes this and I get all the nice type hints, but then when I save xstate cli tool / vscode extension just removes it

mjsampson avatar Apr 12 '23 17:04 mjsampson

We could accept a PR that would introduce an option to add the desired extension to this. In the future the default would be to just always add .d.ts

Andarist avatar Apr 15 '23 12:04 Andarist