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.

Sign up for GitHub

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

Jump to bottom

Translator tester: Only test URL-specified translators when ready. #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Translator tester: Only test URL-specified translators when ready. #21

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions testTranslators/testTranslators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,7 @@ async function init() {
allOutputView = new OutputView();
allOutputView.setDisplayed(true);

const whenReadyToRun = [];
await Promise.all(TRANSLATOR_TYPES.map(async displayType => {
let translatorType = displayType.toLowerCase();

Expand Down Expand Up @@ -474,7 +475,7 @@ async function init() {
// get translators, with code for unsupported translators
if(!viewerMode) {
let translators = await Zotero.Translators.getAllForType(translatorType, true);
haveTranslators(translators, translatorType);
whenReadyToRun.push(haveTranslators(translators, translatorType));
Copy link
Member

Choose a reason for hiding this comment

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

This works, but it isn't very good IMO to depend on non-obvious promise behavior.

When you call a function that returns a promise, the code in that function starts executing immediately. It only pauses on the first await. In this case, it awaits pretty early on and we don't await anything in this function between here and the new Promise.all(), so everything works as expected, but that's pretty fragile. If haveTranslators() should only run once translatorTestViewsToRun is ready, we shouldn't call it at all until that's the case.

So instead we could do something like

whenReadyToRun.push({ translators, translatorType });

and below:

for (let { translators, translatorType } of whenReadyToRun) {
	await haveTranslators(translators, translatorType);
}

(That change also makes the different translator types run sequentially instead of in parallel, which is probably more what we want here.)

Copy link
Author

Choose a reason for hiding this comment

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

How about reverting this change, and simply do (pseudo-javascript)

return or await haveTranslators(translators, translatorType);

on this line? This too works, and it's shorter.

I think here, it's not that haveTranslators() has to run after translatorTestViewsToRun is ready. Actually haveTranslators() is the function that makes translatorTestViewsToRun ready by populating it.

The problems in init(), on the line 428 with await Promise.all(... big arrow function), is that the arrow function creates runaway promises. In each loop in .map() the async arrow function calls the promise-returning haveTranslators(), forgets about it, and immediately returns (a promise that resolves to) undefined. So the await Promise.all() doesn't wait for the promises that do the actual work, only the promises that initiate the work (results of calling the arrow function). That's why this await Promise.all(...) resumes prematurely.

}
}));

Expand Down Expand Up @@ -531,6 +532,7 @@ async function init() {
translatorBox.appendChild(lastP);

// Run translators specified in the hash params if any
await Promise.all(whenReadyToRun);
runURLSpecifiedTranslators();
}
}
Expand Down Expand Up @@ -650,4 +652,4 @@ function serializeToDownload(e) {
e.preventDefault();
}

window.addEventListener("load", load, false);
window.addEventListener("load", load, false);