Refactor. Translate 2.0

[adomasven]

After having a brief attempt at uncluttering the messy code in this codebase while trying to fix zotero/zotero#3182, I have decided that it is pointless.

The actual translate flow is actually quite straightforward, but it is massively complicated by everything being super stateful, and execution flow being controlled by tens of flags set externally and internally.

There shouldn't be a need to instantiate any object when translating, instead, you should choose your Translate (Web, Export, Import, Search), then call translate(), detect() (for Web only?) and getTranslators(). 99% use cases would simply call translate(). If you pass in no translators into translate(), then it runs detect() before translation. If you pass in no translators into detect(), then it runs getTranslators() before translation.

All methods in current translation architecture that begin with set*() should become parameters that are passed into the execution calls above.

Events/handlers should likely disappear from translate at all. Translate should be strictly concerned with returning metadata from a given translate object. Any further processing like item saving or progress display should be performed by the translate calling code. This is simple to do in practice, since most of the saving functionality is already done in other classes per current architecture, specifically Translate.ItemSaver, which all environments that use Translate override.

I propose rewriting the translate code from scratch and providing it as a separate file, then gradually exchanging all existing codebases depending on Translate to use the new architecture, and finally phasing out Translate 1.0 after some years and allowing others to switch over.

This would probably be a huge timesaver going forward, since every bug we encounter in translation, and every time we need to change something in the logic usually means a huge amount of time spent analyzing this complex piece of code and further debugging when a change produces unexpected behavior.

CC @dstillman @AbeJellinek @zoe-translates

[AbeJellinek]

Things we've discussed:

• We'd need to keep the child translator API exposed to translators unchanged. That will mostly be straightforward (it's already implemented as a wrapper over the real translation API), but we'll either need to keep setHandler() in the top-level translation API or implement it for child translators as a shim over options to translate().
• Might need to track pending promises with a waitUntil-style API to support legacy callback APIs (doGet/doPost/processDocuments/child translation), might not, depending on whether we allow simultaneous child translators.
• done could be replaced by the translate() promise resolving.
• error could be replaced by the translate() promise rejecting.
• debug and select could be functions passed as options.
• itemDone and collectionDone could be replaced by a async generator functions or kept as callbacks.
• Export translation needs noWait mode, but export translators are sort of weird: they don't yield items, have a bunch of methods that aren't available to other translator types, and are rarely called from other translators. Do they need to be translators at all?

Other potential considerations:

• We should keep RemoteTranslate in mind while designing the new API. For the most part, if the API is stateless, it should work fine.
• translatorTester.js will need to be updated to use the new API, and maybe deserves a refactor as well. Scaffold uses it to generate item diffs, sanitize items for display, run all test types, and create web tests, but it doesn't create import/export/search tests, so Scaffold implements those itself. We could incorporate it into Scaffold (and remove it from the Connector), make it handle all test types, and add a command-line option to the client that lets you run translator tests headless. That way we wouldn't need Selenium for translator CI anymore.

[adomasven]

Case in point d7299a7

I've spent a lot of time troubleshooting this and trying to come up with solutions, which in the end turned out just removing error handling there completely. translate.complete(false) is an absolute disaster of the current codebase. E.g.:

translate/src/translation/translate.js

Lines 1683 to 1697 in d7299a7

	_detect: async function() {
	// there won't be any translators if we need an RPC call
	if(!this._potentialTranslators.length) {
	this.complete(true);
	return null;
	}
	try {
	await this._loadTranslator(this._potentialTranslators[0]);
	return await this._detectTranslatorLoaded();
	}
	catch (e) {
	this.complete(false, e);
	}
	},

Before the above change this._loadTranslator() may have called this.complete(false), effectively meaning that the current translate operation should be terminated, but since it wasn't throwing, L1692 continued happily executing, to make matters worse, for Import translators, _loadTranslator() is also overloaded:

translate/src/translation/translate.js

Lines 2352 to 2357 in d7299a7

	Zotero.Translate.Import.prototype._loadTranslator = function(translator) {
	return Zotero.Translate.Base.prototype._loadTranslator.call(this, translator)
	.then(function() {
	return this._loadTranslatorPrepareIO(translator);
	}.bind(this));
	}

So the Base.prototype._loadTranslator() calls this.complete(false), but then this overloaded codepath continues executing, and then the codepath in _detect() also continues executing.

---

A bug like this shouldn't even exist in the first place if the code was architectured correctly, but even if it did, with proper execution flow figuring out the issue and fixing it should be a matter of minutes, not hours.

[AbeJellinek]

Horrifying!