Skip to content
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

Svelte allow cross unit compilation #1327

Closed
wants to merge 3 commits into from

Conversation

hgiesel
Copy link
Contributor

@hgiesel hgiesel commented Aug 10, 2021

To allow for cross-compilation unit functionality, and fix the missing reactivity, we need a way to allow for the sharing of the variables in node_modules/svelte/internal/index.mjs.

I've applied a patch using patch-package, which basically does this, where input is the file:

const svelteConstPattern = /^const (.*?) = (.*?);$/gmsu;
const globalIdentifier = 'globalThis.svelteCrossUnit';

output = input.replace(
    svelteConstPattern,
    (_match, identifier, value) => `const ${identifier} = ${globalIdentifier}.${identifier} = ${value}`,
);

and prepended globalThis.svelteCrossUnit = {}; to the file.

The add-on (in this case https://github.com/hgiesel/anki_new_format_pack), needs to be compiled using the esbuild plugin https://github.com/hgiesel/svelte-cross-unit, which I've written myself today.

It basically transforms "creating new state" to "accessing the global state": const a = x; becomes const a = globalThis.svelteCrossUnit.a;.

After doing this, I could already make use of WithShortcut in the add-on, which didn't work before. What do you think about this approach? It doesn't feel right, but then again, I can't think of anything else. The scope of index.mjs is quite small, and relevant state can be found by grepping through the file with ^const (^let).

P.S.
There are also some let declarations in index.mjs, which could be shared across compilation units. However proxying them won't be quite that easy (as every occurence needs to be changed).

@dae
Copy link
Member

dae commented Aug 10, 2021

I feel uneasy about this - I fear it will make updating to future Svelte versions more painful, and with no word from them on if this is a use case they intend to support, there's a risk they'll refactor their code in the future and make such an approach impractical. Before we go any further down this path, could we spend a bit of time talking through the problem and our options?

Firstly, to make sure I'm understanding correctly, is the following correct?

  • The binding issue is limited to bind/let usages in components imported at runtime, and it does not affect the use of stores and contexts?
  • The binding issue only breaks when binding across compilation units - so using a dynamic foreign component that binds foreign subcomponents is fine, it's just when crossing the compilation unit barrier that bindings don't work?

If we provided our component Svelte files in a way that they could be easily compiled as part of an add-on, would that resolve the binding issues here?

@hgiesel
Copy link
Contributor Author

hgiesel commented Aug 13, 2021

Like I mentioned, it also doesn't feel right to me.
After testing out stores and contexts, it seems like stores are the only thing that reliably works. Context is not passed down at all:

Screen.Recording.2021-08-13.at.17.19.48.mov

You can also see how bindings are recognized after a store update happened.

Concerning dynamic components, I'm pretty sure they work, even though I haven't given them a thorough test. It's all about accessing the same state. However users could still not write their own Svelte components this way, they could only match and mix ours.

After looking at the video above, my assumption is that if we provided the components as a library (let's say an anki-components package on NPM), that might resolve the binding issue, but probably not the issue with contexts. However Svelte 3.39.0 also introduced getAllContexts, which would allow us to pass all context as a single prop. Maybe with those things combined, we can provide a "good-enough" solution here.

@dae
Copy link
Member

dae commented Aug 15, 2021

I feel most confident with stores, and think we should be using them as much as possible in API boundaries. As you know, they were specifically designed for interaction with non-Svelte code (meaning more possibilities for users who don't want to use Svelte), they don't rely on compile-time logic, and they have a small API surface, minimizing the chances of something breaking.

I don't have a lot of experience with contexts. Using them across an API/compilation boundary feels a bit risky, though it might be worth testing to see whether getAllContexts() + passing them as a prop solves the problem or not. If it does, and it's practical for users to include the components in their project (either via npm or copying the files in with a script), maybe we'll be ok?

@hgiesel
Copy link
Contributor Author

hgiesel commented Aug 18, 2021

I've played around with the "directly including Anki svelte files and exporting contexts to add-ons" approach, and the biggest issue is indeed the contexts. Doing it this way, will require some boilerplate in each "add-on component", like this:

<script>
  import { setContext } from "svelte";
  
  export let contexts;
  
  for (const [key, context] of contexts) {
      setContext(key, context);
  }
</script>

When referencing the Anki components from the add-on, I couldn't get them to bundle correctly with esbuild, as it was bundling one node_modules/svelte/runtime.js for my defined components, and one node_modules/svelte/runtime.js for the Anki components, which means they didn't share the same state yet again.

To really test it, I would need access to an actual Anki NPM package.

Another issue will be how the anki NPM module would look like. If we want to expose, e.g. WithShortcut, this will lead to lib/shortcuts.ts, which includes lib/i18n.ts, i18n_helpers, tslib, etc.. Most of the code should be able to be tree-shaken however.

@dae
Copy link
Member

dae commented Aug 19, 2021

Good point about i18n etc being pulled in - yuck. Not really keen on having to publish a package on npm either, and the possible issues version conflicts may introduce there.

For our own needs, Svelte seems to do a pretty good job, but it feels like we're really swimming upstream by trying to use it in an add-on scenario, and I fear the problems will only continue in the future. It would have been nice for add-ons to be able to reuse all of our code, but I'm starting to fear that may end up being too fragile / require too much engineering effort to achieve.

Some alternatives I can think of:

  1. Expose the Svelte stores, leaving add-on authors to implement their own UI. Maximum flexibility and minimum effort for us,
    but extra work for add-on authors, and will likely lead to inconsistent UI.
  2. Like 1), but also expose some css for getting similar styling.
  3. Perhaps 1) could be partly addressed by providing some JS functions for common components, like your original JS-based API. My objection to it at the time was it was forcing all of Anki's UI code to be written like that as well, but here I'm suggesting that the JS API be supplementary, and not used by Anki itself, and not necessarily cover all components. Do you think such an approach would be feasible, and not too time consuming?

Any other ideas?

We've gone through multiple iterations trying to find something workable at this point. I know how frustrating it is to write code that ends up getting discarded, and I'm sorry this has dragged out so long. :-(

@glutanimate
Copy link
Contributor

Don't want to derail the discussion, but just to make sure I understand things correctly: Does all of this mean that it would be better to steer away from using any Svelte-based add-on APIs as they could all be subject to change soon? I.e. I was considering porting some of my first add-ons over to the new deck options screen using the respective UI components and perhaps start consuming the new editor toolbar API. Would I be better off staying clear off that for now?

Also, appreciate you guys trying to find a solution that's amicable with add-on development, but I can't help but feel like all of this sounds like quite a bummer overall. Especially when considering how trouble-free working with Qt was. From a purely add-on dev POV, it just feels like performing fairly simple UI tasks has become a lot more complex. And with very little in terms of documentation and examples to go on, both specific to Anki, but also Svelte as a whole, it does feel like there's a huge entry barrier now where there once was a lot more clarity.

@hgiesel
Copy link
Contributor Author

hgiesel commented Aug 19, 2021

@dae
Yeah, it's quite frustrating. Especially because it wouldn't take that many changes for Svelte to support this approach.

  1. I think Shorten CSS references for @import or @use #1332 is the first step towards this. I've been meaning to port webview.css to SCSS as well anyway.

  2. I could give this a go and see how well it works. A reason why I originally encouraged to use it for Anki as well, is because some Svelte features, like slots and event listeners, don't work with this approach. It will need some composability components to really get going with this, e.g.

// OnClick.svelte
<script>
    export let onClick;
    export let component;
</script>
<svelte:component this={component} on:click={onClick} />

// Slotted.svelte
<script>
    export let slotted;
    export let component;
</script>
<svelte:component this={component} />
    {slotted}
</svelte:component>

@glutanimate You should probably stay away from the Svelte API for now, yes.
I agree it's a real bummer, but I also see it as a chance. Extensibility through Web APIs is the only possible way, we could ever get to add-ons which work across all Anki platforms, even though it's still way off.

Regarding Svelte, I find the Svelte docs to be quite insightful, and the tutorial is also very good. The lack of documentation for Anki is somewhat intended, at least from my side, as, well, the approach is flawed. I've developed New Format Pack, as an easy functional example, and wanted to extend from there, however that's also when I noticed the issues, so that's why I stopped there.

@hgiesel hgiesel mentioned this pull request Aug 20, 2021
@dae
Copy link
Member

dae commented Aug 23, 2021

@hgiesel I think we may have missed the obvious here. Instead of hacking Svelte to expose the global state, it looks like we just need to ensure all compilation units are using the same Svelte module, which we can do by importing it at runtime instead of bundling another copy in with the add-on: ankitects/svelte-bug-example@74be242

@hgiesel hgiesel mentioned this pull request Aug 23, 2021
@dae
Copy link
Member

dae commented Aug 24, 2021

Superseded by #1340

@dae dae closed this Aug 24, 2021
@hgiesel hgiesel deleted the sveltecrossunit branch October 20, 2021 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants