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

Support API v3 #8

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Support API v3 #8

wants to merge 12 commits into from

Conversation

inukshuk
Copy link
Member

Resuming from #7

@martimpassos
Copy link
Contributor

I can't figure out this error. The expansion does happen when importing manifests, it seems specific to the tests...

@inukshuk
Copy link
Member Author

The expansion always happens, it's not specific to the test.

The issue may come from the v3 context we have in the repo (not sure if that's the latest version?) or the fixture, but like this the northwestern manifest can't be parsed/expanded.

@inukshuk
Copy link
Member Author

When I print out the manifests upgraded from v2 to v3 they are just empty objects too. Was there something missing in the original PR maybe?

@martimpassos
Copy link
Contributor

I meant the error seems specific to the tests, since the expansion works fine when importing via Tropy:

Screen.Recording.2024-09-16.at.14.25.28-480p.mov

My tests fail to parse any manifests, including the Bodleian one (line 32):

 1) IIIF Manifest
       fixtures
         "before all" hook for "are manifest instances":
     jsonld.SyntaxError: Invalid JSON-LD syntax; invalid scoped context.
      at api.process (/Users/martimpassos/dev/tropy-plugin-iiif/node_modules/jsonld/lib/context.js:403:19)
      at async api.expand (/Users/martimpassos/dev/tropy-plugin-iiif/node_modules/jsonld/lib/expand.js:211:17)
      at async jsonld.expand (/Users/martimpassos/dev/tropy-plugin-iiif/node_modules/jsonld/lib/jsonld.js:325:18)
      at async Manifest.parse (/Users/martimpassos/dev/tropy-plugin-iiif/src/manifest.js:223:20)
      at async Context.<anonymous> (/Users/martimpassos/dev/tropy-plugin-iiif/test/manifest_test.js:32:19)

@inukshuk
Copy link
Member Author

Yes, it just fails parsing any v3 manifest (probably because of the context). Disregard my earlier comment about the empty objects, the upgrade was just missing an await there.

In any case, if it works in Tropy it's likely because of the json-ld library (the one in Tropy uses a few hacks). So it might be due to the version in the tests here fetching some object referenced by the manifest which the document loader in Tropy ignores or the other way around.

@martimpassos
Copy link
Contributor

Do you have any suggestions or plans on how to address this? I tried asking the community about issues with v3 and jsonld lib but didn't get any answers...

@inukshuk
Copy link
Member Author

Right, sorry, I haven't looked into it yet. I remember that we patch some things in the jsonld library particularly in their network stack, mostly because it makes bad choices in the electron context (basically it tries to determine whether its in a browser or nodejs and picks different modules to handle http connections). If this works fine in Tropy that's most certainly the reason why tests fails - so in that case we'd just have to make sure to use the same patches here in the plugin test infrastructure and the plugin is fine as is already.

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.

2 participants