Create one way to handle network dependencies (validator, etc.) #378
Comments
|
Proposal: every module that requires runtime data takes an optional constructor argument
|
I don't like this. It has several bad implications:
Instead, I think we should assume the data comes as a string. It's not the module's responsibility to fetch this data, it's the caller's, so we can move all the fetch logic to a util. |
|
My proposal / intent to implement after discussing with @ithinkihaveacat earlier:
The utility method in
Let me know your thoughts. |
|
@fstanis Sounds good to me! |
|
We should get this in soon for the 1.0 release. |
|
The only thing we should make sure of is to maintain the oneBehind behavior is a default. |
|
I'm OK with ensuring the modules that use it keep using it (we can add it as an option to the new method), but I wouldn't make it the default. Fetching a potentially stale (older than max age) resource doesn't seem like an intuitive thing to do. I think the default should be "fetch if older than max age". |
sgtm |
|
@fstanis Your proposal goes further than I was thinking, but I think I like it. I don't completely follow what the code would end up looking like though--will the classes be exposed at all? Is it like this: // Option A (class exposed to callers)
import {Validator} from "@amproject/toolbox-validator"; // Validator is a class
const validator = await init("https://cdn.ampproject.org/validator.js", Validator); // init() is generic
validator.validate("<html>...</html>");? Or this: // Option B (class hidden from callers)
import {validatorInit} from "@ampproject/toolbox-validator"; // validatorInit() is a function
const validate = await validatorInit("https://cdn.ampproject.org/validator.js");
validate("<html>...</html>");? Supporting functions: // @ampproject/toolbox-validator
class Validator {
data: string;
constructor(data: string) { // no network access!
this.data = data;
}
validate(s: string) {
return true;
}
}
async function validatorInit(urlOrString: string) {
const data = await fetch(urlOrString);
return new Validator(data);
}
// @ampproject/toolbox-core
async function init<T>(urlOrString: string, klass: new(...args: any[]) => T): Promise<T> {
const data = await fetch(urlOrString);
return new klass(data);
} |
AMP has some dependencies on a few resources hosted on
cdn.ampproject.org: a document is valid iff the validator hosted atcdn.ampproject.orgsays it is, the definitive source of runtime version comes from this host, etc.For various reasons, it's not always a good idea to actually fetch these resources every time a module is invoked. (e.g. if attempting to validate multiple files it's not feasible/reasonable to fetch the validator for every file that needs to be validated; running tests should be possible without network access.)
Various AMP Toolbox modules (and friends) have created different approaches to dealing with these problems, see:
We should probably come up with a single mechanism for handling these use cases.
/cc @Gregable @sebastianbenz @fstanis
The text was updated successfully, but these errors were encountered: