[Compiler] Thoughts about a custom interface for inklecate

ephread commented 2 years ago

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:

has a pretty barebone interface;
lacks a command that makes it easy to identify from external tools – such as a "version" option that returns a successful exit status.

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

Usage: inklenode <command> [<args>]

The inklenode commands are:
   build           Build the given story
   play            Play the given story
   stats           Print statistics about the given story
   lsp             Start the language server

Options:
   -V, --version   Print the version
   -h, --help      Print the quick help

See `inklenode help <command>' for information on a specific command.
For the full documentation, see: https://github.com/y-lohse/inkjs

Build

Usage: inklenode build [-cvj] [-o <filename>] <file>

Compile the given ink story into the JSON intermediate representation.

Options:
   -o, --output=<filename>    Output file name
   -c, --count-visits         Count all visits to knots, stitches and weave points, not
                              just those referenced by TURNS_SINCE and read counts
   -j, --json                 Print errors and warnings in JSON (for communication with tools like Inky)
   -v, --verbose              Verbose mode; print compilation timings
   -h, --help                 Print this message

Play

Usage: inklenode play [-jk] <file>

Play the given ink story. <file> can either be a valid JSON file or an ink file.

Options:
   -j, --json                 Output playthrough in JSON (for communication with tools like Inky)
   -k, --keep-running         Keep inklenode running in play mode even after the story is complete
   -h, --help                 Print this message

Stats

Usage: inklenode stats [-j] <file>

Print statistics about the given story, including word count. <file> can either
be a valid JSON file or an ink file.

Options:
   -j, --json                 Output the statistics in JSON (for communication with tools like Inky)
   -h, --help                 Print this message

Language Server Protocol (Future)

Usage: inklenode lsp [--node-ipc|--stdio|--socket=<number>]

Starts the language server.

Options:
   --node-ipc                 Use node's IPC to communicate with the language client
   --stdio                    Use standard I/O to communicate with the language client
   --socket=<number>          Use the given socket number to communicate with the language client
   -h, --help                 Print this message

Version

$ inklenode --version
inklenode 2.1.0
Compatible with: inklecate 1.0.0

What do you think?

smwhr commented 2 years ago

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. (You said nothing about inklenode adding the BOM and I still dont know where I stand on that !)

ephread commented 2 years ago

Shouldn't we promote this discussion in the inkjs channel of inkle's Discord ?

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).

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)

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.

(You said nothing about inklenode adding the BOM and I still dont know where I stand on that !)

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.

y-lohse commented 2 years ago

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. We can keep the code in this repo, but we might have to shuffle things around a little to turn this into a monorepo. However a possibly simpler option would be to build this as a standalone tool, on top of the javascript compiler?

(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.)

ephread commented 2 years ago

(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.

smwhr commented 2 years ago

Inklecate is just a cli wrapper around the Compiler.Compile + a cli Story loop, I'm not sure it needs its own repo, at least for the build/play parts. Many package deliver their code along with binaries to use them easily and make them available through npm run shortcuts. (And I know many split them also when they get big enough)

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)

y-lohse commented 2 years ago

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 PosixFileHandler too. Personally I don't see any harm in exposing more classes publicly, it gives users more options but it won't break anything.

Many package deliver their code along with binaries to use them easily and make them available through npm run shortcuts.

That's true as well. This discussion is related to https://github.com/y-lohse/inkjs/pull/931#issuecomment-1045951719 . My main goal is that:

Users that are unfamiliar with javascript can keep using inkjs as a story player as frictionless as possible — no need for build chains, build chain configuration, etc. Import a js file and you're up and running.
Advanced users have the flexibility they need. They get the browser support that they require, they have some control over how much code gets shipped to their users, etc

The suggested architecture does respect these points so that's all good.

So the choice, i think, is:

Expose a simple CLI based compiler+player as part of the inkjs package
Want to keep it separate / anticipate that it will grow into it's own substantial project

If we go with 1, I don't even think we need to compile the current inklecate.ts file. We should be able to point the bin field of the manifest to that file, and voila. npm run inkjs mystory.ink would then invoke inklecate.ts.

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. If we go with 2, I can help out with the setup but I realistically wouldn't become a regular contributor due to lack of time :(

danbri commented 1 month ago

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...

smwhr commented 1 month ago

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 re-export from inky using the latest inky and you should be fine.

danbri commented 1 month ago

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 https://github.com/y-lohse/inkjs/issues/933#issuecomment-2266664046, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABJSGKTMRP3H62RGAGVBBLZPSUPVAVCNFSM6AAAAABLRHNH4OVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRWGY3DIMBUGY . You are receiving this because you commented.Message ID: @.***>

y-lohse / inkjs