Set output format after driver initialization

[adomasven]

Could we have a setOutputFormat() API if it doesn't require reinitializing something fundamental in the driver? We can adjust Zotero code to only ever set output format on driver initialization but we might have plugins that are used to calling setOutputFormat() available on citeproc-js and we don't want to break them unnecessarily.

[cormacrelf]

These plugins own their own instance, right? If so, the format change doesn't need to be temporary, it can happily invalidate large swaths of the cache. I believe this is what citeproc-js does.. So yeah, I can do that for you.

As I am sure I have detailed before/elsewhere, the invalidation requirement is a disambiguation thing -- if the output format is plain, and you have a style that uses italics to differentiate between e.g. books and journal articles, then you want to ignore the italics for disambiguating 'Jones, Title, 1999' and 'Jones, Title, 1999'. In plain mode it should go further and make them 1999a/b or whatever step comes next. In order to change what happens here you have to invalidate large parts of the computation graph. Because I knew this was going to be the case, I didn't even put the format in the dependency graph, but it will be pretty easy to change the implementation of the "get format" function.

(Either that or just manually wipe, which I think would also be pretty easy since it's all in the one section of the db. In the terminology of the graph citeproc-rs uses (Salsa), if it's in the graph, the output format should be set with "high durability" so the engine can avoid checking a node whose dependencies are more durable than the highest-durability change since last check. This is nice, because the format presumably does not change often, but nearly every node in the graph depends on it.)

You would have noticed that the previewCitationCluster API already has a custom output format option. This can be "incorrect" in the way I described, which you can observe by asking for plain output when the internal format is HTML, or vice versa. This doesn't actually warrant consternation, because it's a faithful preview of what would actually appear in the document, just in another format. So it's actually more correct than rebuilding the whole thing from scratch for every single preview call.

[adomasven]

I'm not creating bridge functions for processCitationCluster() of citeprocJS, so that might still introduce incompatibilities with our plugin developers. If this too much of an undertaking you shouldn't prioritize it too highly.

[cormacrelf]

No no, I think it'll be fine. Lots of text != lots of code :)

If plugin devs are depending my JS API, I suppose that means we need a 1.0 release for it right? There are a couple of things to clear up before I do that:

• Main thing: I could finally crank out the wasm-bindgen modifications that would eliminate the .unwrap()s and make the glue code do it for you. (Other users of Rust wasm-bindgen have expressed an interest in this as well.) This would be a pretty big breaking change if done after it's in the wild.
• Smaller: I wanted to change the cite list argument to the preview function to be an actual Cluster, which can provide e.g. cluster mode args, and any future extensions.

[cormacrelf]

Oh, also semi-related, the free() API would be required of any plugin authors still. FF ESR 78 doesn't have "weak refs" aka auto free so Zorero-bundled citeproc-rs can't use it until ESR 91. Calling free when weak refs are enabled doesn't break anything so that can be added later and the free() call can be made optional.

I am not familiar with the plugin API, but if it has lifecycle events like "unloading plugin" then that would be where to put free() calls.

[adomasven]

If plugin devs are depending my JS API

They are not yet, but as we start transitioning to using citerproc-rs they will, although mostly indirectly via our citeproc-js bridge functions. Setting the output format after driver initialization however is something that some might be used to doing.

Main thing: I could finally crank out the wasm-bindgen modifications that would eliminate the .unwrap()s and make the glue code do it for you. (Other users of Rust wasm-bindgen have expressed an interest in this as well.) This would be a pretty big breaking change if done after it's in the wild.

I have wrappers around all API calls to citeproc-rs that automatically handles this, so this isn't a big deal for us, but this is a change that certainly should happen before any stable releases.

• Smaller: I wanted to change the cite list argument to the preview function to be an actual Cluster, which can provide e.g. cluster mode args, and any future extensions.

Seems like a good change.

I am not familiar with the plugin API, but if it has lifecycle events like "unloading plugin" then that would be where to put free() calls.

There's not any plugin API at the moment, plugins monkey patch Zotero functions and use XUL overlays. This isn't the current "industry standard" (or even close to it) for plugin APIs, but it was at one point for Firefox and thus Zotero too. We might start adding some dedicated plugin API functions, but that's a separate discussion. Either way, not many Zotero plugins use citeproc independently and the one I looked at uses Zotero codepaths to get a citeproc instance, so they'll see little effect from all of this. However needing to unload citeproc is something that we should make sure they are aware of.

I suppose that means we need a 1.0 release for it right?

Yes, it would be nice to lock in citeproc-rs APIs, since we'll soon start testing it with our users.