Improved add-on extension API

[hgiesel]

This PR does two things:

Streamline editor design:

• Rename OldEditorAPI.svelte to NoteEditor.svelte. There was a NoteEditor.svelte before, but it was only some CSS, so I merged it in. By now I think it's more maintainable if a Svelte component starts with at least one HTML element, because it facilitates CSS debugging, as you can easily look up the elements you see in the dev tools.
• Rename currentField and activeInput to focusedField and focusedInput: They both rely on focusin/out events, so it makes more sense this way.
• Remove $focusInRichText context variable: The same can be done be achieved by inspecting focusedInput.

The big feature of this PR is sveltelib/component-hook, which allows to dynamically attach to the onMount functionality of svelte components (if we allow it).

import CodeBlock from "./CodeBlock.svelte";
import { onNoteEditorMount } from "anki/NoteEditor";

onNoteEditorMount(({ toolbar }) => {
    toolbar.formatInlineButtons.appendButton(
        { component: StrikeThrough },
        2
    );
});

You can even inspect all currently mounted instances of one component. I use these methods in qt code as well.

The PR is still a draft so far, because I have to rewrite the API what happens you execute appendButton (it should be bases on promises, because currently the function is assigned yet, when NoteEditor's onMount executes).

There are two questions/ideas I take away from the PR so far, and they both have to do with naming:

1. How should we name the exports that come from a .svelte file. Should we keep the .svelte suffix, and have it look like this:

import { onNoteEditorMount } from "anki/NoteEditor.svelte";

2. Looking at this code, it is a lot of work just to rename things:

const {
    setContextProperty,
    get: getNoteEditor,
    has: hasNoteEditor,
} = contextProperty<NoteEditorAPI>(key);

const {
    setupComponentHook,
    onMount: onNoteEditorMount,
    onDestroy: onNoteEditorDestroy,
    instances: noteEditorInstances,
} = componentHook<NoteEditorAPI>();

export { getNoteEditor, hasNoteEditor };

registerPackage("anki/NoteEditor", {
    getNoteEditor,
    hasNoteEditor,
    onNoteEditorMount,
    onNoteEditorDestroy,
    noteEditorInstances,
});

Maybe we should export/register all the api methods as one api object:

const [setupContextProperty, contextProperty] = contextProperty<NoteEditorAPI>(key);
const [setupComponentHook, componentHook] = componentHook<NoteEditorAPI>();

export { contextProperty };

registerPackage("anki/NoteEditor", {
    contextProperty,
    componentHook,
});

It might also reduce the confusion a bit. getNoteEditor e.g. only works in a svelte context, which could be easy to forget/oversee with a generic name like this. componentHook should probably also be plural:

import { componentHooks } from "anki/NoteEditor";
componentHooks.onMount((api) => { ... });

It might also be easier for communication. We could say: NoteEditor supports componentHooks and contextProperty, whereas EditorField supports contextProperty, and add-on devs could rely that the contextProperty functionality from both of them functions similarly, and gain familarity with the interface. Otherwise we'd have to say: you can use getNoteEditor from anki/NoteEditor, or getEditorField from anki/EditorField, and add-on devs must trust that the similarity in naming implies similar functionality, or read the source code.

Another advantage: We could initialize them as objects of a class, rather than through closures. I've abstained from classes for them so far, because I didn't want to use object.method.bind(object) the whole time (and using bind is actually worse for performance than closures).

[dae]

Maybe we should export/register all the api methods as one api object:

registerPackage("anki/NoteEditor", {
contextProperty,
componentHook,
});

Please bear with me here, I don't have a strong handle on this code yet. :-)

Is my understanding correct that we'd also need to export the key symbol, so the caller can extract the properties from the context? Could you give a brief example of how it would look from the calling end?

I am a bit wary about exposing Svelte contexts in our API, as keeping the usage of contexts internal gives us more flexibility to refactor things in the future. But if we have a leaky abstraction that depends on there being an active context anyway, maybe we're not gaining very much from hiding the context away.

Ultimately you know this code better than I do, so I am happy to defer to your judgement here.

import { onNoteEditorMount } from "anki/NoteEditor.svelte";

If our exported packages will always have a file of the same name/path backing them, then that seems reasonable, as it will improve discoverability. I seem to recall you mentioned previously about moving more logic outside of the Svelte files into TS files though - maybe that's another way to solve this that would avoid the need for the extension?

[hgiesel]

Is my understanding correct that we'd also need to export the key symbol, so the caller can extract the properties from the context?

Ahh, that might be true if we use objects, at least until private class fields have wider support. Keys should certainly stay private. But we could still export it as a closure.

// MyButton.svelte, which needs to be mounted on, e.g., the EditorToolbar
import { contextProperty } from "anki/NoteEditor.svelte";
import { editingInputIsRichText } from "anki/NoteEditor.svelte";

let disabled = true;

if (contextProperty.has()) {
    const { focusedInput } = contextProperty.get();
    disabled = editingInputIsRichText(focusedInput);
}

I am a bit wary about exposing Svelte contexts in our API, as keeping the usage of contexts internal gives us more flexibility to refactor things in the future.

That is true, however as soon as we want to allow add-on devs to mount their own Svelte components, it almost becomes necessary. The button component needs to figure out whether a field has focus :). We still keep some flexibility too: we could export two contexts, one for internal, one for add-on usage. Add-ons also still cannot directly import things we only export, but not pass to registerPackage.

I seem to recall you mentioned previously about moving more logic outside of the Svelte files into TS files though - maybe that's another way to solve this that would avoid the need for the extension?

I already did that for one file: editable/ContentEditable.svelte, which led to editable/content-editable.ts. I found this to be helpful to separate the "logic" from the "component initilization" code. However there are some APIs that will always be exported from .svelte files, e.g. the aforementioned contextProperty.

[dae]

if (contextProperty.has()) {
const { focusedInput } = contextProperty.get();
disabled = editingInputIsRichText(focusedInput);
}

Couple of thoughts here:

• Shame about Allow variable declaration in if condition statements microsoft/TypeScript#14309.
• Instead of 'contextProperty.has()', what about 'context.available()'?
• (Getting a sense of deja vu - apologies if I've raised a similar issue before) - could editingInputIsRichText be a method on focusedInput, instead of as a separate stand-alone function? From an API perspective, I find a handle that provides various methods on it to be more discoverable/ergonomic than the C-style pass-handle-to-separate-functions approach. But...

Add-ons also still cannot directly import things we only export, but not pass to registerPackage

I guess that would be a downside of having methods instead of external functions - presumably we couldn't easily separate them into public and private, without using naming such as internalXXX(), or switching from contextProperty() to some other proxy object that feeds the allowed methods back into the real context.

[hgiesel]

could editingInputIsRichText be a method on focusedInput

The definition for it is the following:

export function editingInputIsRichText(
    editingInput: EditingInputAPI | null,
): editingInput is RichTextInputAPI {
    return editingInput?.name === "rich-text";
}

So technically the user could just check the .name themselves. I just thought it would be nice to hide some implementation details.

I find a handle that provides various methods on it to be more discoverable/ergonomic than the C-style pass-handle-to-separate-functions approach.

I agree, especially if you have good IDE support. However I have structured the NoteEditor to be oblivious to the existence of RichTextInput and PlainTextInput as much as possible.

So to say focusedInput (and EditingArea.svelte in general) is just an open interface, for other components to program against.

That's why I've structured RichTextInput more as a "built-in" add-on.

I haven't done so for no reason. It might be thinking too far ahead, but what e.g. if we wanted to have Markdown support for fields, then we'd build a MarkdownInput, a/o MarkdownPreview, which, I imagine, would be in the same place where now RichText and PlainText is. Or what if we wanted to add built-in Image occlusion support. We'd have to make EditingArea aware of all those different input types.

I guess that would be a downside of having methods instead of external functions - ...

Yes. Even though if we keep those "API objects" very narrow in scope, we might not even get in the situation of "we want to have this method externally available, but not this one".

Off-topic: Another thing I though of today when I was fiddling with my "New Format Pack" add-on, was that we assumably could have a anki-addon.d.ts, which would provide types for all of those registerPackage calls we make. I don't think we'll be automate that, but I think it would provide a lot help for add-on devs.

[dae]

That's why I've structured RichTextInput more as a "built-in" add-on.

👍

I don't think we'll be automate that, but I think it would provide a lot help for add-on devs.

Agreed on it being helpful, but handling transient types seems like it might be tricky? If we have a public API that is returning an object of a type defined in one of our other source files, presumably we'd either need to repeat that definition in the addon.d.ts file (yuck), or the add-on authors would need access to other types somehow - either by distributing our other type files as well, or by attempting to bundle them using something like https://github.com/Swatinem/rollup-plugin-dts

[dae]

I think this change may not be correct. svelte.ts executes inside Bazel's build structure, which contains a bazel-out folder. The recent rename was just of the convenience symlinks, which are only used for accessing Bazel's files outside of Bazel. Code that runs as part of the Bazel build process uses the internal tree, which still uses the old names.

Work/code/dtop% ls -l .bazel/dtop/bazel-out
drwxr-xr-x  - dae 23 Jan 15:27 _actions
drwxr-xr-x  - dae 26 Jan 21:16 _tmp
[...]

[dae]

(could you rebase over the latest main when you have a chance?)

[hgiesel]

I've got it to work - thanks.

Having .d.ts file(s) for add-on developers actually seems to be something that apps do: here's the NPM module for add-on developers for the Obsidian note-taking app. It's nothing but a .d.ts file that's included via packages.json's types property.

It works quite well in my small experiment. I get full code completion with working links:

Sadly I don't think writing the definition files can be automated easily, because we artificially fabricate them by calls to registerPackage. You can also do type imports from .d.ts files, which means we could import all the base types and then replicate them:

// NoteEditor.d.ts
declare module "anki/NoteEditor" {
    type ContextProperty<T> =
        import("../../sveltelib/context-property").ContextProperty<T>;
    type LifecycleHooks<T> =
        import("../../sveltelib/lifecycle-hooks").LifecycleHooks<T>;
    type NoteEditorAPI = import("../../editor/NoteEditor.svelte").NoteEditorAPI;

    export const context: ContextProperty<NoteEditorAPI>;
    export const lifecycle: LifecycleHooks<NoteEditorAPI>;
    export const instances: NoteEditorAPI[];
}

For packages which contains function, we'd have to import their arguments' and return value's types, or we create explicit types for the definition files to import:

// file.ts
function ourFunction(a: ArgumentType, b: number): ReturnType { /* ... */ }
export type OurFunctionType = typeof ourFunction; /* only for option 2 */
registerPackage("anki/ourPackage", { ourFunction });

// ourPackage.d.ts: Option 1
declare module "anki/NoteEditor" {
    type ArgumentType = import('../../lib/file").ArgumentType;
    type ReturnType = import('../../lib/file").ReturnType;
    export function ourFunction(a: ArgumentType, b: number): ReturnType;
}

// ourPackage.d.ts: Option 2
declare module "anki/NoteEditor" {
    type OurFunctionType = import('../../lib/file").OurFunctionType;
    export const ourFunction: OurFunctionType;
}

[dae]

Not much experience with d.ts files, so I might need to dig into this a bit more on Tues. Just a quick one for now, re

    type ContextProperty<T> =
        import("../../sveltelib/context-property").ContextProperty<T>;

I presume the import() is required to allow relative imports. If the imports weren't relative (eg we used "paths" in tsconfig.json, just for this), would it allow us to simply the code to something like the following?

    export { ContextProperty } from "@sveltelib/context-property"

[hgiesel]

Hm, I wasn't able to have the .d.ts file accept path aliases from tsconfig. Just one point: We'd want the types available as global/ambient type declarations.

Something I tripped upon is that in .d.ts files you switch the mode to a module / module augmentation, once you use an import or export statement (not an import expression like I did in NoteEditor.d.ts).
If you don't use any import/export statements, you make a global declaration, with which you can type the environment the code lives in, and can make ambient values, like globally accessible variables or importable modules.

E.g. if you have the global declaration file:

// file.d.ts
type ContextProperty<T> = import("../../sveltelib/context-property").ContextProperty<T>;

// code.ts
let a: ContextProperty<number>; // OK

You can use ContextProperty in your code, without ever importing it, it just exists as a global type. But if you make a declaration module:

// file.d.ts
export { ContextProperty } from "../../sveltelib/context-property";

// code.ts
import type { ContextProperty } from "../../anki/ts/typings/common/NoteEditor"
let a: ContextProperty<number>;

Then you have to import the type first, before you can reasonably use it.

[dae]

I don't have enough experience with the ts ecosystem to know what is common/best practice, so I may be wrong here, but isn't adding types to the global namespace a bit messy? Some brief Googling yields posts like https://www.reddit.com/r/typescript/comments/bm5yks/is_defining_an_ambient_module_for_appwide_types_a/

[dae]

It looks like Figma (by the esbuild guy) doesn't use ambient types in their API: https://github.com/didoo/figma-api

[hgiesel]

isn't adding types to the global namespace a bit messy

I agree. I would suggest to only use declare module globally. This would mean that users can use statements like import * as NoteEditor from "anki/NoteEditor" and get back a typed object.

For types I would suggest that users still import them directly from our TS code. While that might break with us moving code around, it will not invalidate the add-on code (it will still compile). Having access to the types directly is mostly just necessary for typed callbacks.

[hgiesel]

I've heavily refactored the mechanism for dynamic component insertion, and was able to contain the code better in its own components, rather than bleeding into unrelated components.
E.g. if you compare the ButtonGroup.svelte, you'll see that it now contains way less code, while that code is now contained in DynamicallySlottable.svelte, Item.svelte, ButtonGroupItem.svelte, and sveltelib/dynamic-slotting.ts (and also contains some documentation in there). I hope that this makes it more explicit for everybody.

While it looks better now, the API is still experimental, with some things to improve, but for now I'd like to concentrate on other things.

[hgiesel]

I don't like the extra indentation at all, but it makes the approach considerably easier to understand imo. (and also to remove from a component, without having to dig into subcomponents)

[dae]

🚀

[dae]

Could you test this when you have a chance? I won't hold up the merge on it.

[dae]

Thanks, the docstrings are quite helpful :-)

[dae]

I've heavily refactored the mechanism for dynamic component insertion, and was able to contain the code better in its own components, rather than bleeding into unrelated components.

Looks like an improvement, thanks!