-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Compiler] Thoughts about a custom interface for inklecate #933
Comments
Shouldn't we promote this discussion in the inkjs channel of inkle's Discord ? I have no idea how a lsp work but, I sense it may rely on the parsedState and its tree populated with debugMetadata and I should warn you, there may be some work left to do to be iso with the C# code.(but the parsedState is easy to expose) As for the rest, I agree with you. |
Eventually, yeah, that's a good idea! I want to create the last issue (about floats) before I start harassing Yannick as well, so he'll get the full context. With the weekend coming up I think I'll be able to write it tomorrow night. I also need to create the poll in a separate issue (probably with https://gh-polls.com/ or something similar).
You're on point, if you're curious you can take a look at the partial C# implementation. It worked fairly well! The LSP is a long-time goal, though, I think we'll focus on the other aspects first.
That's true, that's because I'm going to bring up the topic of a "compatibility" mode on the issue I have yet to create. That said, I think the suggestion you made on #931 hits the sweet spot. Let's have inklecate/inklenode add the BOM for now, it doesn't hurt anyone and it ensures maximum portability. |
The proposed architecture makes sense to me, the separation seems very natural and I don't have any objections in terms of changing the behavior from inklecate as this tool wouldn't be meant as a 1:1 replacement. My main point would be that this should probably be a separate package? We wouldn't want to ship this as part of the "normal" inkjs package since it's only useful in some specific contexts. (i'm also not entirely sure that the lsp should be built into the CLI compiling tool, to me they feel like quite different use cases, but I don't feel strongly about this.) |
I think you're right. I got caught up in "the old way". I attempted to add it to inklecate because it was convenient; the C# runtime and the compiler weren't available as separate packages. But it'd be very easy to have a separate tool and simply depend on inkjs. tsserver is a separate executable. +1 on having inklecate distributed separately. |
Inklecate is just a cli wrapper around the To be able to do more substantial work in an outside repo, will we have to export big chunks in the npm package ? (Never done that before) |
Yeah we would have to export whatever classes are needed by consumer packages. In the case of the current "inklecatejs" implementation this would mean exporting the
That's true as well. This discussion is related to #931 (comment) . My main goal is that:
The suggested architecture does respect these points so that's all good. So the choice, i think, is:
If we go with 1, I don't even think we need to compile the current If we go with 2, we'd want to export more classes (which we can do regardless), and create a new repo for that. It does represent a fair amount of work. I'm good with either option. If we go with option 1 we may have to refuse PRs to the CLI tool in the future if we feel like it adds too much maintenance work. |
Is this still planned? I am trying to parse .ink at runtime to use from js, and ran into a version mismatch between js web player template vs non-js inklecate. It looks like I missed an easier approach... |
Inky was updated recently with the latest version of inkjs. An older one was lying around for far too long that, indeed, could only interpret story up to version 20. |
Awesome, thanks!
…On Sat, 3 Aug 2024 at 11:14, smwhr ***@***.***> wrote:
Inky was updated recently with the latest version of inkjs. An older one
was lying around for far too long that, indeed, could only interpret story
up to version 20.
Just reeport from inky using the latest inky and you should be fine.
—
Reply to this email directly, view it on GitHub
<#933 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABJSGKTMRP3H62RGAGVBBLZPSUPVAVCNFSM6AAAAABLRHNH4OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRWGY3DIMBUGY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Since we are soon going to have a functional compiler in the main branch (yay \o/ 🎉) with its own inklecate (double yay \o/ 🎉), I want to bring up something that has been on my mind for a while.
The original inklecate:
The reasons behind 1. are easy to infer; inklecate contains zero dependencies, which makes argument parsing difficult and inkle probably never needed to support long arguments. As for 2., there's an opened issue on the main repo, but it hasn't seen much activity.
I think we have an opportunity to make some improvements (IMHO – this is, of course, relative) and, in full honesty, solve some of my pain points. 😅 I've thought about discussing the topic and creating a PR for upstream in the past, but I realised pretty quickly it would end up increasing the maintenance burden for inkle.
Here's what I have in mind:
I'd like to split each of inklecate's main features into separate subcommands with separate sets of options (expressed both through short and long flags), see below.
Since the interface is going to be different, I'd like to use a different command name to prevent confusion. (The name is pending, but I've used inklenode below.)
Both the runtime and the compiler would remain free of external dependencies, but inklenode would have a tiny depency on a good argument parsing library. I can live with that given it's not meant to be embedded in a webpage.
Command Interface
Main
Build
Play
Stats
Language Server Protocol (Future)
Version
What do you think?
The text was updated successfully, but these errors were encountered: