-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Improving dynamic TS package ergonomics #1764
Comments
If we get this working, it means we can also drop the separate NoteEditorAPI import, as onMount will be typed properly. That makes things a bit easier for add-on authors to pick up, especially if we return the object without destructuring: // import type { NoteEditorAPI } from "@anki/editor/NoteEditor.svelte";
NoteEditor.lifecycle.onMount((editor): void => {
// editor.focusedField. etc...
editor.toolbar.inlineButtons.append({ component: StrikeThrough, id: "strikeThroughButton" }, 2);
... A logical next step after that will be to look into providing these typings without having to build Anki. I wonder if it's as easy as copying all our source+generated d.ts files into a folder? |
True, I've noticed that before as well. We also have a components variable for the deck-options. As a design choice I wanted to make sure that I think you can see why I hesitated here :)
I'd say we're still juggling the types, but in Regarding "@anki/runtime", I'd prefer to avoid things that break the illusion of using a normal library. There's also one other "player" when it comes to typing: In https://github.com/ankitects/anki/blob/main/ts/lib/runtime-require.ts#L7 I left a remark regarding typing |
As an experiment, I added the components exported by the editor to the deck options page. The difference in bytes was quite small: 434406 That will obviously grow as we add more components, though as you know, any components that the page happens to be using will have already been included, so we'd only be paying for the ones we don't use. Provided we limit that package to commonly-used, lightweight components, maybe it would be simplest to export them as anki/components on all pages? Currently the editor is stuffing context keys into the components, and exporting the i18n keys as a separate global; perhaps we could place the context keys into the editor package instead? Do editorModules need exporting? For the deck options screen, its components are ones specific to that screen. Maybe we could export them as a separate package dedicated to the deck options?
With our current bundling process, if a given page exported multiple components, we'd pay the same cost in file size whether they were split into multiple runtime packages or a single one. With some combination of modules in the browser and tweaks to our runtime handling we could perhaps get benefits here, but it's a bit of an unknown (to me at least :-), and I'd be tempted to take a YAGNI approach here.
Do we gain anything from separating out the editor-specific components from the rest of the editor exports? My instinct would be to place them together, and have a single package for each area/page of our app. For example, the deck options page could export 'anki/deck-options' with something like: interface DeckOptionsPackage {
// page-specific components
components: {
TitledContainer: TitledContainer,
SpinBoxRow: SpinBoxRow,
// ...
},
instances: [DeckOptionsPage],
// lifecycle, etc
} Would that work? Re our current 'anki/NoteEditor' package, would it be worth maybe matching the folder name instead, to make the packages a little easier to locate in the source code, eg 'anki/editor'? |
Yes, technically you are right :-) But it's somewhat less cumbersome in the Svelte file:
(I realise now I should have written: const noteEditorPackage: NoteEditorPackage = {
context,
lifecycle,
instances,
}; in my earlier example instead of using
I don't have strong opinions at this point, and am just exploring ideas, so please don't interpret the following as me pushing strongly for this. I just want to understand your position better, as I may be overlooking something. I was thinking that "anki/runtime" (didn't intend to put a @ there) could function as the single entrypoint into all the items we make available at runtime. I was imagining it exporting functions like the following: import type { NoteEditorPackage } from "../editor/NoteEditor.svelte";
/**
* Exports from the editing screen. Only available on pages that show an editor.
*/
export function editor(): NoteEditorPackage {
return require("anki/NoteEditor") as NoteEditorPackage
}
// import the various components by value, ensuring they get pulled in to
// all of our bundles. Could do the same thing for other shared items like i18n
import IconButton from "../components/IconButton.svelte";
export interface SharedComponents {
IconButton: IconButton,
LabelButton: LabelButton,
// ...
}
/**
* Basic Svelte UI components. Available on all pages.
*/
export function components(): SharedComponents {
return { IconButton, LabelButton, ... };
} (Side thought: I miss microsoft/TypeScript#39930) I see a few possible advantages with this approach:
import * as anki from "anki/runtime";
const editor = anki.editor();
editor.lifecycle.onMount(...)
const { IconButton } = anki.components();
...
In terms of potential downsides I can see:
Are there other various downsides that I may have overlooked? One thought while looking through this area:
If we were to go down the runtime file route, some of that may not be necessary - end-users would get the typings we defined in runtime.ts, and we don't use the runtime exports in our own code as far as I'm aware. We would still want to make sure that the package names we're registering are the same ones used in runtime.ts, but one alternative way to do that would be to have a package name exported as a constant at the same location as the package interface, and we could then reference it in runtime.ts without the need to maintain a centralized list in runtime-require.ts. Interested to hear your thoughts. |
One big advantage I see with typing in .d.ts is less complexity in the .svelte files. The whole typing is only for the benefit of add-ons, but it would bloat up Anki's .svelte files in return.
Regarding "anki/runtime": However this would still need a registerPackage("anki/runtime", {
editor,
components,
}); And it would also mean that there's now multiple "paths", to access the same package: require("anki/NoteEditor") === require("anki/runtime").editor()
However, we'd still need to maintain one bigger .d.ts file, which would pull in types from all over the ts directory. I guess this might be personal taste in the end, but I'd prefer smaller packages, with more cohesion.
I don't quite understand how it would be different in terms of casting from smaller modules? It would still need an ambient module declaration in the form of a .d.ts file.
Wouldn't it then also try to find "anki/runtime" under the alias? I feel like there's some misunderstanding here, either on my or on your side 🤔 Regarding the "illusion": My point here is that it would be convenient for the add-on developer, if he doesn't need to understand how we package our libraries, but can just treat it like any other typescript project. The point with the unit tests is fair, though. At that point, it would have to be communicated.
Currently there's no "anki/editor". The idea behind naming the package "anki/NoteEditor", is that it is a "component package". So my idea was that we have two kinds of packages:
I also realized that I kind of broke that naming scheme already, because "anki/TemplateButtons" exposes something more akin to a library function, or maybe it should have been But I think it's a nice guideline, to be able to say "I want to modify component X, so I can start with |
I've made a quick proof of concept to show how I was thinking it might work: Responding to some points (running out of time today, so forgive my brevity)
Is it that excessive? We're not doing much except explicitly stating the types of our exports, and I feel like that actually improves readability / makes it easier to see at a glance what we're making available to add-on authors. I do understand the desire to keep the add-on support stuff separate. But we already have to include add-on exports and support code in our code anyway, and I feel like keeping the types in the same place will make maintenance easier for us than having to maintain the files separately - when we want to add a new export to this screen, we add one line to the object literal, one line to the interface directly above it, and we're done. I think we should be doing our best to provide complete and correct types. They offer a few advantages:
With that goal in mind, I feel like having the types in the Svelte file makes more sense - there's less room for forgotten/mistyped/stale entries, and we don't have to jump back and forth between multiple files.
My thinking was that if we went ahead with this plan, we'd expose the editors to runtime.ts in some other way, so they can't be accessed by end-users except via anki/runtime. While looking into api-extractor, I came across this page: https://api-extractor.com/pages/setup/configure_rollup/ The section "An important limitation" seems relevant to the discussion here.
Luckily it looks like we can automate that :-)
The d.ts generated by api-extractor has some nice properties:
Let's come back to the naming issues later :-) |
Sorry, ran out of time last night. So, on the naming issues, we are currently flattening our exports into a single level, eg lib/bridgecommand → anki/bridgecommand By doing so, we're discarding the logical groupings between our exports, and this feels a bit jumbled. What if we tried to maintain some grouping instead? For example, if bridgecommand and shortcuts are something we wanted to make available on every page, we could export them in a 'lib' or 'library' const in runtime.d.ts, similar to what I did with
What confused the issue for me somewhat is we have two kinds of components - small ones intended for instantiation by add-on authors in their own code as well, and large ones like NoteEditor that are basically the entrypoint/manager for a page or a large part of one. Add-on authors are more likely to use these large components only for the API they provide - they will mostly be interacting with the existing instances Anki has provided, instead of making their own. That caused me to see NoteEditor as more of an abstract 'handle to controlling the editor' than a component in itself for these API exports, and that was why I opted for the name 'editor' there. It does obscure the fact that it is a component though, and I can see how you might find that unclean/misleading. To keep things neater/maintain grouping, one option would be to place it inside an editor package, eg export const editor = {
components: {
NoteEditor: require("anki/NoteEditor") as NoteEditorPackage,
// ... any other components we want to export that are specific to the editor
},
} Then the add-ons would be using Some variations we could take: a) We could add an One downside of such approaches though is we're splitting our exports up into multiple locations - NoteEditor is exporting parts of the API, and runtime.ts would be pulling in parts from editor/ to construct editor{}. I think it might be clearer/easier for us if a given high level component has all of its exports in one place, for the same reason placing types next to the definitions makes things easier. d) In addition to lifecycle/context/etc, NoteEditor could export a
Do you mean exposing the functionality as a global instead? I'd really like to see us move as close to 0 globals as possible.
I agree that consistency and being able to infer the correct API call easily are important, and I want that too :-) With the d) approach, wouldn't we get something similar, except the group would be included too?Eg for components ts/editor/X require("anki/runtime").editor.components.x.lifecycle.onMount() But for the "top level" components where this is going to be the most common, there would be a shortcut: require("anki/runtime").editor.lifecycle.onMount() |
Answers to the second post
However grouping keeps the same risk as paths in import strings do: It makes refactoring harder. Currently I try to move things to For example, you mentioned "SpinBox[Float] in deckOptions.components".
As we use slots heavily here, that wouldn't be quite that easy. I think that's something we should really only do once there's good demand for it. It probably makes always sense to expose some more closer-to-metal components, like It wouldn't really make sense to expose them to add-ons, also because they are linked via the context api's to components below and above. Generally we can probably say, that we shouldn't expose components to add-ons, that use
From those options, I'd prefer It's hard to say, what is the "top" component, as we're working our way up. At some point a
Well, yes, but I mean as exposing to Anki, but not add-on devs. The idea is kinda similar to when you use underscore variables in Answers to the first post
That sounds good. Something I'm wondering, in the second post you said you want some grouping, but you also want to put everything into "ts/runtime/index.ts". Couldn't we create multiple API entry files, under "ts/runtime", and then make the add-ons types point directly to the runtime directory? E.g. a "ts/runtime/editor.ts". (even though I'd prefer "ts/runtime/note-editor" here, as it include would everything from NoteEditor down). You linked the api-extractor homepage, which mentioned that we should avoid path-based imports, but this grouping, as you also noticed is not really path-based here. |
As I understand it, the problem with invoking API extractor on multiple entrypoints is that TypeScript will treat the types in each type bundle as distinct. If add-ons only ever targeted one screen that would be fine, but if an add-on wants to target multiple screens, common types like DynamicSvelteComponent would be problematic, as code dealing with them would expect the type that came from its own bundle, not a bundle for another page.
Yep, that's a valid point. We can partially mitigate it by being conservative with what we export (waiting until we're fairly confident it will stick around in that place). And we do have the option of deprecation warnings+aliases like you have in runtime-require.ts. If everything were exported at a single top-level namespace without any grouping, then it avoids the problem of a page-specific component from becoming more general. But such an approach is not without downsides - it's messier, and users won't be able to tell at compile time whether a given component will be available or not. One other alternative would be an inverted approach: no 'global' components, and each page exports the components it wants to make available in a flat namespace, spreading in the standard ones, eg IconButton would be in both deckOptions.components, editor.components, etc. SpinBox would be in deckOptions.components but not editor.components for now; if we later promote it to a 'standard' component then it would be available on all pages. Downsides of doing it this way include the fact that it's hard to tell what is standard vs what is page-specific, and the fact that everything will tend to be lumped together in a single flat namespace for the page.
100% agreed; I'm in no rush to do this, I just thought it was one of the reasons why you were objecting to an 'editor' export.
I take your point there, though I'm not sure that fully invalidates this approach. I was thinking of NoteEditor as the 'top' component of our editor area, which is shared among the different screens, much like how add-ons can extend Editor directly, or target AddCards/Browser if they wish. Add-ons that wish to alter the way the editor behaves everywhere will want to be able to make changes to the editing component, instead of having to make changes in the browser and add screen separately.
Whether we're doing it for add-ons or for our own uses, don't globals still feel a bit messy compared to, say, exporting a variable from one module that others modules can import and mutate?
Say an add-on wants to pop up a dialog, generate some media file based on user input, and then insert the media into the field that was focused prior to the dialog popping up. Is there a way they can they do that currently, aside from using the legacy addMedia() routine which we've labeled 'legacy'? This API stuff is a hard problem :-) I'm not sure there's any perfect solution here, and the challenge is just figuring out which approach is least bad. |
Re #1626 (comment), I've been hunting for a way to make this a bit less cumbersome & more robust than manually declaring the exports. What if instead, we export the module's interface in the same place as we register it, ensuring that the interface matches what we're exporting? Eg
Add-ons could then use require() with a cast to get a typed module:
I spent some time trying to make this work in the context of a 'declare module' so we could keep the standard TS-style import, but couldn't figure out a way to spread the interface into the module definition except by doing so manually:
At least we're not juggling types that way, but it's still extra work. Maybe there's some way I'm not aware of?
If not, perhaps one other option would be to define a .ts file that does the require/cast for this and other modules and exports the resulting symbols, so add-ons could just do something like the following?
One other thing I noticed while looking at hgiesel/anki_new_format_pack#11 is that we're currently exporting
components
as a global. Could we be stuffing that into a runtime module somewhere instead to make it more discoverable, and typed?The text was updated successfully, but these errors were encountered: