Citeproc RS support #2220
Citeproc RS support #2220
Conversation
| // where citeproc_rs_wasm_include.js attaches some classes to Zotero.CiteprocRs. | ||
| // This works fine until we call resetDB() in the test runner (or any other time we | ||
| // could call ZoteroContext.reinit(). A usual require() call returns exported symbols | ||
| // from some Gecko script cache. Unfortunately that means that the necessary citeproc | ||
| // classes do not get reattached to the Zotero object and the Citeproc Wasm driver breaks | ||
| // when trying to create its own objects (e.g. Zotero.CiteprocRs.WasmResult). |
There was a problem hiding this comment.
Is there anything I can do to help avoid this? It's been a while since I came up with that solution, maybe worth a revisit if this is what's come of it. What happens in a reinit/resetDB?
There was a problem hiding this comment.
I think the real solution would have been to provide a citeproc-rs commonjs build that was a little bit more backwards compatible with older runtimes, such that the custom Zotero build with the hacky attaching wouldn't have to be necessary. When I was loading citeproc-rs initially the modifications required to the include script were rather minimal and I think would have avoided this situation too. On the other hand this works in the meantime and we'll be moving to a newer version of Firefox in the intermediate future which will hopefully allow us to use the standard wasm bindings builds, so it will solve itself naturally.
There was a problem hiding this comment.
Does running the test suite detect the issue reliably? If so I can try the old way with your old patches back in, and see if it goes.
There was a problem hiding this comment.
You can add await resetDB() before https://github.com/adomasven/zotero/blob/9bc9f22fbcf1f04d04df9c0aa3313bb0bfc0aafa/test/tests/citeprocRsBridgeTest.js#L54 and then run runtest.sh -d 5 citeprocRsBridge which will break the test if you replace the loadSubScript() line above with the commented out require('citeproc_rs_wasm_include');
| // Don't try to parse author names. We parse them in itemToCSLJSON | ||
| citeproc.opt.development_extensions.parse_names = false; |
There was a problem hiding this comment.
I actually have a name parser as well, do you need an option to disable it?
There was a problem hiding this comment.
Unfortunately the person who added this change is gone from Zotero and there's no note for why citeproc name parsing was disabled, as there doesn't seem to be any obvious reason or conflicts that this would create. I also couldn't find anything on citeproc-js github when the option was added, nor in our issues. I don't think it's necessary now, unless perhaps @dstillman knows something here.
There was a problem hiding this comment.
Yeah, I don't know. As long as the tests in 1cbd7f7 still pass, I guess we're good until someone tells us otherwise.
There was a problem hiding this comment.
Well, I guess it's about CSL-JSON output and uses beyond our built-in citation processing, right? Does the CSL spec call for specific name-parsing behavior, or was that just something that citeproc-js implemented? I assume the goal here was just generating CSL-JSON for consumers that might not have complex name parsers built-in.
Does disabling the name parser actually make a difference if the CSL-JSON is already pre-parsed?
There was a problem hiding this comment.
Does the CSL spec call for
No
Does disabling make a difference
There is a situation in which it would -- someone uses the "quoted name" method to avoid the name being parsed. The quotes are stripped, the name is slotted in the correct field, and then it would be parsed again.
There was a problem hiding this comment.
Since it's not part of the CSL spec I'm not sure the processor should do it by default. Perhaps it should out of convention, but it should be clearly stated in the docs and yes, there should be an option to disable it.
adomasven
force-pushed
the
citeproc-rs
branch
from
7d30db6
to
9bc9f22
Compare
adomasven
force-pushed
the
citeproc-rs
branch
from
8f4de8f
to
20aa899
Compare
adomasven
force-pushed
the
citeproc-rs
branch
2 times, most recently
from
497dd1b
to
20aa899
Compare
|
Once we update this with the line-spacing fix, I think we can go ahead and merge this. |
|
Should we merge this? |
|
I will update citeproc-rs once more with the latest changes and then I don't see a reason not to. |
Disabled under zotero.cite.useCiteprocRs by default
…e for citeproc-rs
|
Well, actually, we should wait for @cormacrelf to merge zotero/citeproc-rs#142 first |
|
Nearly there guys, just beating the GH actions into shape. I'm merging and then cutting 0.2.0 I think. |
|
Alright, 0.2.0 is out on NPM! Release notes here with the 3 action items up top. https://github.com/zotero/citeproc-rs/releases/tag/wasm-v0.2.0. |
adomasven
force-pushed
the
citeproc-rs
branch
from
303840c
to
5e96f7e
Compare
|
Now ready to merge |
|
Nice.
… On Dec 15, 2021, at 11:23 AM, Adomas Ven ***@***.***> wrote:
Now ready to merge
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#2220 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB3ZFQVRGL4FUXHRYZQOPTURBUDDANCNFSM5FYDB3QQ>.
Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Adds experimental citeproc-rs support. Disabled behind a hidden
cite.useCiteprocRspref. @cormacrelf only published canary builds on NPM, so this isn't ideal in terms of NPM dependencies. But he has recently said that he'd be publishing a stable release. Also seems like my NPM got updated and this added this whole mess of a v2 package-lock file thing, which seems to be a transitory stage to a new format. Idk if we want that or not.Related #1845