API improvement ideas

[dabbott]

Overall the compiler API works well, but there are a couple things that I think could be improved to make it easier to figure out when diving in for the first time:

1. I found the names parseWorkspace and parseFile confusing, since they don't necessarily parse anything (e.g. if everything I need is in helpers already) and they do things beside parsing too (e.g. write files).

   Maybe convertFile and convertWorkspace?

2. I found it confusing that the return value of parseWorkspace would differ by plugin - the permissive return type of Promise<any> makes it hard to guess what value it will return.

   I think these APIs should return something more restricted (like Promise<void>) for consistency, and if a plugin wants to expose other methods, that's fine. E.g. the tokens plugin could have a createFlatTokens() -> Promise<ConvertedWorkspace> of some kind, and parseWorkspace will be a thin wrapper around that.

3. I found it confusing that the return value of parseWorkspace would change based on the output option.

   I got tripped up by this for a bit when trying to generate a temporary flat tokens file in the android plugin - it's hard to infer this is the behavior just by looking at the types.

   One approach for configuring output that could work well is to have plugins write all files to an in-memory FS, and only write the in-memory FS to the real FS in bin.ts, based on the output option. This would have a few advantages:

   • We can easily support other variations of output:
     • a --dry-run option where we print a helpful diagnostic message showing a name/snippet of all files that will be written, useful for choosing config options and debugging plugins
     • a JSON-map output where we print the whole file tree as JSON for easy consumption by other tools
   • Plugin authors don't have to handle a missing output option, it'll get taken care of automatically
   • If plugins call each other, or if we allow running multiple (e.g. both js and swift) at the same time: we can track which plugins wrote which files, by adding a separate layer to the in-memory FS for each plugin. A plugin could also be called as a temp intermediate step.
   • Prevent a partially-generated workspace on disk if a plugin fails
   • Easier to test the plugins, since they're side-effect free
   • Easier to import the compiler in browser and other environments

   I was playing around with the in-memory fs packages that webpack uses, and wrote this proof-of-concept to validate that these packages did what I thought: https://gist.github.com/dabbott/a7c23da95b90ef34374144ff5d61670e. We'd need to fiddle around with the details, but so far I'm liking this direction.

4. We're probably gonna need a way to write binary files soon, for image assets. It looks like we only have a helper for string files at the moment.

[mathieudutour]

1. Yeah sure, I think I used parse because I started with the tokens and it really felt like a parser

2. That would prevent us to use lona convert for the tokens (eg. we would have to require the plugin directly). parseFile really shouldn't return void because the main use case for it is to not write to disk. While it can be confusing if you use any plugin, as long as you use a specific one, you will know (because it exports the right types).
   It's also really helpful when you use the node API with a specific plugin:

   const tokens = await lona.convert('workspace', 'tokens') as ConvertedWorkspace

   which would be a lot trickier if you need to parse a map of the files or something.

3. While I agree that an in-memory FS would be nice (and the architecture is already in place for it), it doesn't solve most of it IMO:

   • plugin authors already don't have to care about the output option, just use the fs helper. The only plugins that do are tokens and documentation because we want to print to stdout a single string. It's an edge case and shouldn't matter for other plugins.
   • the plugins are already side-effect free. You need to inject an in-memory fs helper (that's not going to change whether we use one in the compiler or not)

   But otherwise yes, the dry option and JSON map output would be nice to have.

4. 👍

[dabbott]

2. a. Maybe parseFile -> Promise<string>?

   b. I'm wondering if parseFile should be an optional API (on the Plugin type definition)? Now that tokens files can contain multiple types of tokens and are interconnected through variables, there often isn't really a 1:1 relation between input and output files.

   c. IMO we shouldn't recommend an approach that casts an any using as, even if it's convenient, since that will throw away type safety. Since tokens is somewhat of an edge case (almost all other plugins will probably want to invoke it), maybe we export a method directly from lona? Or expose a helper? I would be using that from android if it existed, although I also don't mind importing a method from the tokens plugin directly.

3. a. I initially injected the in-memory FS helper, but then I started thinking, I would probably want all plugins to buffer their output by default. Is there a downside to making it core?

   b. What makes the documentation plugin an edge case?

[mathieudutour]

If you force to return a string, then you pretty much for people to do JSON.parse and you end up with a typecast anyway (that's what you have to do if you shell out to the compiler: JSON.parse(execSync('lona convert tokens')) as ConvertedWorkspace)

We could make it parseFile<T> -> Promise<T> but it wouldn't matter when you use lona.convert(x, 'tokens') since the plugin is dynamically required so there is no way to know its type statically.

But I think we are talking about 2 different use cases:

1. use the compiler
2. using individual plugins directly in other plugins

So for 1., we are talking about the public API of the compiler, and I think it needs to be any to allow all those usages I talked out.

For 2., I don't mind exporting an additional method with a stricter type when there is no output. But IMO plugins shouldn't require core plugins otherwise they become public API and we have no way of knowing what is going to break if we make internal changes.
Plugins should only use the helpers passed by the compiler. If you feel like all plugins will need the evaluated tokens (I'm not sure that's true but that's a different story), then we should expose them in the helpers.

---

3. Probably no downside to adding a in-mem FS helper, but it should be both reading and writing to be consistent. It should also be used to get the lona config, etc.. It's a pretty big change and until now it didn't feel really necessary, more like a nice to have eventually.

What makes the documentation and tokens an edge case is that they are a format designed for our internal use, and both output a single object consumed by our tools. The user isn't supposed to use them.
Eventually, the tokens format should disappear IMO, and be replaced by specific plugins (sketch, figma) - and the documentation plugin should live in the documentation github action doc repo.
If we do want to expose those formats, then we will need to be a lot more careful.

[dabbott]

I think what makes this a tricky challenge is that the compiler does a lot of different stuff. A lot of this stuff is "just for now" while some of it might last longer. We invoke it from the command line and also directly require it in different JS/TS projects. We use it to: write arbitrary files for the end user, generate miscellaneous JSON formats, generate intermediate data types for other JS/TS projects, and probably more. It also has some roots in the Reason version (which itself had its roots in the original JS version), which I added onto here and there as-needed, rather than having a strategy from the start.

Maybe if we take a step back, we can think of a better approach for some of these things. I like your point about tokens/docs intermediate formats being internal. The way I see it is, right now we should mainly focus on our public/user-facing workflow & API:

• Our public API is the compiler's command line interface for converting a workspace's input files to output files
• Our public input formats are anything that sits in the workspace: our markdown, logic, and lona.json formats
• Our public output formats are the JS, Swift, Kotlin, Docs bundle, Sketch, and Figma

I'd say everything else is internal. What do you think?

I see the compiler plugin API as something like... the level directly below the compiler's public CLI. Now's a good time to iterate on it, since it might be one of the next things we try to solidify and make public. The other use cases for the compiler (via JS/TS) are important, but I see that as internal for the foreseeable future, so I think don't think they should influence the plugin API.

If I think about how this affects the plugin API: I want to support this use case of converting a workspace's files super well, so that we can provide a good user experience.

• I'd say the most important thing is a solid convertWorkspace (parseWorkspace) API that takes the input files and "writes" the output files (we could write the files, return a serialized version of the FS to be written later, etc). That should be required for all plugins, and we shouldn't overload it for other purposes, e.g. we shouldn't return a ConvertedWorkspace:

  compiler/src/plugins/documentation/index.ts

  Line 48 in 5113dc0

  ): Promise<ConvertedWorkspace | void> => {

  I think this return type is the main reason I thought to invoke a plugin from another plugin, but I agree that we shouldn't do that through this API.
• I do want to be able to get the flat tokens data in some internal way. I'm not sure if it's common enough to warrant a helper. I don't think they necessarily need to be a plugin. If we need to generate them via compiler CLI, then maybe a plugin is the right move (or it could be a private compiler CLI command) - but it shouldn't affect our convertWorkspace API (i.e. if they are a plugin, then they should consume input files and write output files, even at the expense of an extra JSON.parse). If we don't need to generate them via CLI, then it can just be a method we export.
• I want to be able to do a dry run with a diagnostic summary and avoid partially written workspaces - I see these as things we should add now for all plugins for a better user experience. So I would bump the priority of this.
• I don't see convertFile as a candidate for being a public API yet, since converting individual files isn't a strong user-facing use case (1:1 file conversions are not as important as they were in the old Reason version). So given that this is internal, maybe we don't need to figure out a consistent return type for this one
• Helpers currently expose a lot of our internal format through ast, which is definitely necessary at the moment. I think helpers may be in a fuzzy gray area between public and internal for a while and I think that's OK, so I haven't really considered what should/shouldn't go in there yet

[mathieudutour]

All very good points. Happy to scale down the API to just convert workspaces and add a in-mem fs helper 👍

IMO the compiler is a thin wrapper around plugins. To give as much freedom as possible to the plugins, I don't think the plugin API needs to be strict regarding the return type of convert functions. The compiler can just forward whatever the plugin does while giving it helpers to facilitate it.

Is there a need to actually restrict it? From what I understand you got confused because you tried to call the plugin directly without knowing what it returns.

1. That's my bad for not documenting it
2. I don't plugins should be called directly
3. When you do know what it returns, it's super powerful
4. I can improve the type of the function with an overload (to make it clear that it returns void with an output and a convertedWorkspace otherwise)

Given all of that, not restricting the API opens up a lot of use cases and doesn't affect this use case of converting a workspace's files?

• For the user of the CLI it's all transparent: they know the plugin they are calling so they know what they will get (and can either cast or type guard the result or just ignore the result)
• For the plugin developer they can do whatever they want: either return something (like the list of lint errors?) or write files.

I'm not sure I understand your point about the helpers exposing the AST tho, surely plugins must work with the AST we provide instead of creating their own?

[mathieudutour]

One other thing that comes to mind regarding the im-mem fs is that, as its name suggests, it's all in memory and could become an issue for big workspaces with a lot of assets (we can imagine adding sounds and videos potentially).
So maybe it shouldn't be the default mode but only used with the dry option.

Regarding the diagnostic summary, I don't think it's linked to the in-mem fs: we have a reporter helper that we can improve to bundle all the issues and present them nicely if we want to

[dabbott]

I can improve the type of the function with an overload (to make it clear that it returns void with an output and a convertedWorkspace otherwise)

I don't know how that works in TS but that sounds good, especially if we can make the overload and output flag private (since this use case is only internal at the moment?). I think mostly it's the type of Plugin that matters

Is there a need to actually restrict it?

The way I see it is, suppose we take this argument even further: what if we return any instead of Promise<any>? Using any has some benefits: as long as we only ever call convertWorkspace using await, it'll have the same behavior, and it gives us the option to support even more potential use cases, i.e. converting workspaces synchronously. The main problem with this is that as a plugin author, I now don't know if it's safe to do an async operation in my plugin just by looking at the function signature - so I have to read the docs to figure it out (an explicit rule just became an implicit assumption) and hope they're up-to-date. If we ever decide to change that assumption in a future release, plugin authors won't get a compile-time error. A smaller problem is that it's only a matter of time before we call convertWorkspace internally in the compiler and forget to await.

The same is true of Promise<any> vs. Promise<void>. As a plugin author, one makes me think "I'm supposed to return something... but what?". The other makes me think "Well, I know I'm not supposed to return anything...". Restricting the return type gives the plugin author important clues/guarantees about how it will be used and therefore how to write it, plus we can always relax the restriction later without consequences, so I think we should always start as restrictive as possible.

For the user of the CLI it's all transparent: they know the plugin they are calling so they know what they will get (and can either cast or type guard the result or just ignore the result)

I'm not sure I understand this point. Won't all convertWorkspace CLI calls (so far) write files? Excluding internal things like flat tokens.

For the plugin developer they can do whatever they want: either return something (like the list of lint errors?) or write files.

I think a list of errors is an important use case, but wouldn't that be a Promise.reject(errors)? Do you mean if they're just warnings?

One other thing that comes to mind regarding the im-mem fs is that, as its name suggests, it's all in memory and could become an issue for big workspaces with a lot of assets (we can imagine adding sounds and videos potentially).

True - this is really the only downside that comes to mind for me. I would probably optimize for what I predict is the common case, where this won't be an issue, and have an option to turn off in-memory. We could warn to stderr about this flag and/or automatically turn it off if we see a file that's >N bytes. I see it as akin to turning off a caching mechanism or increasing heap size in a project that gets too big. But I'm not sure, I can see it defaulting to off working too.

Regarding the diagnostic summary, I don't think it's linked to the in-mem fs: we have a reporter helper that we can improve to bundle all the issues and present them nicely if we want to

True. I thought it would be a shortcut but definitely not necessary. I'm thinking something like

I'm not sure I understand your point about the helpers exposing the AST tho, surely plugins must work with the AST we provide instead of creating their own?

I haven't thought about it enough to really have an opinion yet, but I see a possibility of exposing a more structured API of accessing things, with using the raw AST a last resort. The AST is a good format for serializing code, but I'm not sure it's optimal for understanding it. A lot of the time when I want to access a node, I want to know other handy info, like its unified type, evaluated value, where it was defined, what files must've been executed first before it can exist. Maybe we make a hub for looking this stuff up from a node, without having to figure out where each thing comes from and look it up by the node's UUID, for example. Or maybe not, just thinking out loud. Misc thoughts:

E.g.

interface Declaration {
   namespace: string[];
   identifier: string;
   unifiedType: Unification.Type;
   get evaluatedValue: Value;
   owner: LogicFile;
   dependencies: Declaration[];
}

interface LogicFile {
   isLibrary: bool;
   sourcePath: string;
   dependencies: Declaration[];
   declarations: Declaration[];
}

function generateKotlinLogic(files: LogicFile[]) {
  files.forEach(file => {
    const { dependencies, declarations } = file
    dependencies.filter(dep => !dep.owner.isLibrary).forEach(dep => {
      const { owner: { sourcePath }, namespace, identifier } = dep
      // Generate an import using all this stuff
      // Java/Kt often import each class separately
    })
    declarations.forEach(decl => {
       const { name, unifiedType, evaluatedValue } = decl
       // Generate Kotlin decl (other options available for custom functions, etc)
    })
  })
}

[mathieudutour]

Using any has some benefits: as long as we only ever call convertWorkspace using await, it'll have the same behavior, and it gives us the option to support even more potential use cases, i.e. converting workspaces synchronously.

That's true but not really possible since the wrapper around the plugin is itself async (get the config, etc.).

The thing is, a type definition isn't documentation. We shouldn't expect plugin developers to 1. use TS, 2. use the definition.
We should have documentation anyway, and keep it up to date! There is no way people are going to write plugins otherwise.

I don't think it's too far fetched to write something like:

A plugin's `convertWorkspace` method can optionally return an object, in which case the compiler will return it as well (written to stdout when called as a CLI)

A major advantage of the new compiler is its node API. Restricting the output of a plugin to void completely goes against it.
I was actually thinking about going a step further and have compiler.convert also accept a plugin object directly (instead of a string) and infer its return type from it.

Won't all convertWorkspace CLI calls (so far) write files?

Tokens and Documentation do not - and that's the only ones that we use 😄. We can also imagine a "lint" plugin or an "accessibility" plugin which could return a list of warnings based on certain rules - without ever writing to disk.

We could warn to stderr about this flag and/or automatically turn it off if we see a file that's >N bytes.

That could be a good solution - but this entire flag kind of seems like a premature optimization. We could start writing it as a separate thing, to be injected in tests - and only then implement it in core.

Tbh I'm still struggling to understand what is the use case for the in-mem fs for the end user. Sure the dry output looks nice, but it is really useful? What prevents you from just executing it in a separate output folder and looking at it? It will only be marginally faster and it's not costing anything.

I do agree that it would be really helpful for plugin devs - especially if we can provide some fixtures and test helpers around it - but nothing we couldn't implement separately.

A lot of the time when I want to access a node, I want to know other handy info, like its unified type, evaluated value, where it was defined, what files must've been executed first before it can exist

I thought that was what the evaluation context was about? I like that the AST is just a json object. Easy to reason about.
But yeah we can def improve the evaluation context, maybe provide a method taking a node as input and returning all those info

[dabbott]

Hmm... you bring up some good points. I think the challenge here is that we're optimizing for different use cases, and we don't know which is the one we should be optimizing for yet.

Let me just throw this out there as an alternative to our approach with the compiler so far: maybe we invert the control flow and treat the compiler as a library. The compiler could be just for Logic related stuff: scanning the workspace, parsing the AST, creating the evaluation context, etc. That's the core of the compiler - the heavy lifting that I'm certain needs to be shared between commands. Other scripts or libraries can use @lona/compiler to understand a Lona workspace, and we focus on making that easy to use. We could optimize for importing in TS.

Any command that generates stuff can be a separate package that consumes the compiler library. Each package can decide if it wants to generate files and/or be importable via TS. If we want to generate tokens via CLI, run npx @lona/tokens. If it only needs to be importable in TS, then it doesn't need a CLI command at all. Docs: npx @lona/docs. Android: npx @lona/android. Android won't need to be imported by another TS package at all yet, so we don't need to think about exports/returning anything. In @lona/android, I'll import @lona/compiler, @lona/tokens, and prettier, which really isn't that much stuff.

If there's a lot of commonality between specific packages already, e.g. tokens and docs, and we want to share most of it, we could make a @lona/script-helpers and @lona/fs-helpers or something, for parsing args and writing files and whatnot. We could still have a monorepo for all of these core packages, and manage them through lerna to keep versions in sync more easily. Basically, split out the script runner and FS helper from core - it seems like we don't know enough to know what should/shouldn't be common across all "plugins", so enforcing a uniform layer of abstraction isn't helpful yet.

What do you think?

Tbh I'm still struggling to understand what is the use case for the in-mem fs for the end user. Sure the dry output looks nice, but it is really useful? What prevents you from just executing it in a separate output folder and looking at it? It will only be marginally faster and it's not costing anything.

I think you're right that we don't know how helpful it'll be. I predict it'll be helpful since it's something I want to use now, but maybe if the android plugin were already written and mostly bug free I wouldn't use it. Since it's just a hypothesis, maybe it's best if we do it in a specific plugin first and see if it's useful or not. My current workflow is: make sure no staged changes, run the android generation, open my visual git viewer, see what happened based on added/modified files, if it's not what I wanted, git stash, otherwise, git commit.

evaluation context

This piece is only for lazily evaluating Logic nodes, analogous to how the unification context is only for determining types - if we want to make a new interface that encompasses other stuff, let's call it something else and it can refer to the evaluation context.

[mathieudutour]

That's kind of how I envisioned the compiler already: the plugins are in total control are what they are doing, the compiler is just there to facilitate it.

But I do think there is value in it being a wrapper around plugins instead of the other way around:
it's a lot easier to write a function that takes a couple of arguments than a full-blown cli, even if we write lots of helpers for it. You would have a lot more boilerplate, it's harder to update to a new version of the compiler (backward compatibility is easier than forward compatibility), documentation will necessarily be more spread out, etc..
It also gives us the possibility to do some smoke testing: grab random plugins and call them, see if they run without error.

It's also nicer for the end-user: you're sure that all plugins can be called the same way.

But yeah, happy to split the fs from the core if you want (I do think that every plugin will need it tho, even if just reading files).

My current workflow is: make sure no staged changes, run the android generation, open my visual git viewer, see what happened based on added/modified files, if it's not what I wanted, git stash, otherwise, git commit.

Yeah that's my guess. It's mostly a dev tool, I don't think the user will care. So we should write it with this use case in mind. Maybe some helper to compare with a snapshot, etc.

[dabbott]

Hmmm... not sure. It really depends how similar plugins actually are I think. Like, if it were just JS, Swift, Android, I'd be more confident in the compiler being in control and being able to determine a good abstraction. But if we're thinking about tokens, docs, linting, a11y - which all do arbitrary things, I'm not sure if this approach is best. Maybe we just haven't landed on the right abstraction though. Definitely no need to change anything for now though. I don't want to thrash on stuff until we're more sure

you're sure that all plugins can be called the same way.

This is only true if we enforce a consistent return value though

It's mostly a dev tool, I don't think the user will care

I think our users are devs. I mean, hopefully a lot of them will use our CI service, but I'm sure there will be people who want to run the compiler directly and write plugins and stuff, and I think that's a valid use case. And then as soon as you start generating code you end up with all this junk in your repo while you're figuring out how to use the compiler commands. I'm fine with everything being a helper though

---

I gotta make some progress on Android, so I'm gonna forge ahead in that repo and then we can figure out if it makes sense to move stuff to core or later or not

[mathieudutour]

This is only true if we enforce a consistent return value though

No, you can always call them from the CLI, consistently. You can also use the node API, you're just not sure what you get if you don't know the plugin. But you can always call them.

I think our users are devs. I mean, hopefully a lot of them will use our CI service, but I'm sure there will be people who want to run the compiler directly and write plugins and stuff, and I think that's a valid use case

Right, but it's kind of like a framework that has plugins. Take gatsby for example: what's the percentage of gatsby users that create gatsby plugins?

---

Yeah, this is a great conversation to have and to have in the back of the head, but I agree there is no rush to change stuff for now - I already changed the most trivial ones

[elimisteve]

I think these APIs should return something more restricted (like Promise) for consistency, and if a plugin wants to expose other methods, that's fine.

Maybe I haven't gathered enough context yet, but: why not return a Promise<SomeInterfaceImplementedByAllPlugins>? If the plugin wants to expose additional methods or fields it can return a value of a concrete custom type which has them. The caller of the function returning a Promise<SomeInterfaceImplementedByAllPlugins> could always choose to cast this value from the more generic interface type to the concrete if it wants to access those additional details. This way we get both static typing and flexibility.

[dabbott]

An interface would definitely work! I think that'd be a lot clearer/safer than Promise<any>. Right now the only thing all plugins do consistently is write files, although future plugins like linters might not even need to do that. We could type the return value as an empty interface for now though, so we allow the flexibility of custom fields.