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

Add necessary utilities for generic translators #20

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

AbeJellinek
Copy link
Member

Causes a stack overflow due to circular references when being copied into the
sandbox. We should probably make SandboxManager#importObject() deal with
circular references without blowing up, but this works for now.
Copy link
Member

@dstillman dstillman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsonld.js is this? Is this built fresh or just pulled from node_modules/dist/jsonld.js after an npm i jsonld?

In the Connector, do we know if this (large) file gets injected into each webpage/sandbox, or does it just get loaded in the background page/service worker? (@adomasven might be able to comment on this.)

utilities.js Outdated Show resolved Hide resolved
@AbeJellinek
Copy link
Member Author

Built fresh. I could add a script to zotero/zotero to do the build if you'd like. It's essentially just jsonld.js from the jsonld NPM package passed through Browserify with some very minor config changes.

@dstillman
Copy link
Member

Yeah, for a large, modified third-party file, particularly one that has to be included in the browser extensions, it's better when we can point to a build script — e.g., so that we can point to that if a reviewer complains about it. (They really prefer unmodified files, but I guess that's not an option here.)

Should we just make the jsonld package a dev dependency of this repo and add a script to update the file? We're set up to do that in zotero/zotero, but the whole point of this repo and translate is to remove dependencies on zotero/zotero for translation stuff. zotero/zotero-build also has build scripts, but those are all for updating static dependencies in zotero/zotero. Probably clearest to keep this in one place in this repo?

@AbeJellinek
Copy link
Member Author

OK, script added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants