Extract runtime parameters #736
Conversation
|
@mdmower would be great if you could take a look. You probably know most about how runtime version parameters need to be handled. Thanks! |
This PR extracts fetching and configuration of runtime parameters into a separate function outside the transformers. This externalizes and centralizes runtime parameter handling enabling, removing the responsibility of handling runtime configuration inside transformers (with the potential for duplication) and enabling future optimizations of file system based caching of runtime artifacts (see #650).
There was a problem hiding this comment.
I ran this through my own little optimize script (tests ampproject cdn and self hosting, with and without rtv) with the few necessary changes and it works as expected.
Consolidating the read and manipulation of runtime parameters into fetchRuntimeParameters was a good idea, thanks for working on it!
Only a few suggestions to consider, nothing blocking.
| * | ||
| * @param {Object} config - the AMP Optimizer config | ||
| * @param {Object} customRuntimeParameters - user defined runtime parameters | ||
| */ |
There was a problem hiding this comment.
You might consider adding a @returns to quickly identify the parameters that are typically returned. It's a bit difficult to identify them all by reading through the code. Similarly, in the description, you might mention which parameters are simply validated, which parameters are fetched and verified via network, and the rest pass-through untouched.
| @@ -145,13 +144,13 @@ class DomTransformer { | |||
| * @param {Array.<Transformer>} config.transformations - a list of transformers to be applied. | |||
| */ | |||
| setConfig(config) { | |||
There was a problem hiding this comment.
I might opt for this.config = this.setConfig(config); in the constructor so that it's easier to identify that DomTransformer has property config available without digging into a function.
There was a problem hiding this comment.
This duplicates initializing the field in the constructor and in the setter. Imo setX implies that there's a field (my Java roots might be showing trough here...)
| @@ -1,3 +1,9 @@ | |||
| <!-- | |||
| { | |||
| "ampRuntimeVersion": "0123456789", | |||
There was a problem hiding this comment.
Since AMP has decided RTVs must be 15-digits, we might want to get in the habit of using valid versions in examples and tests.
There was a problem hiding this comment.
good point - done
| @@ -1,3 +1,9 @@ | |||
| <!-- | |||
| { | |||
| "ampRuntimeVersion": "0123456789", | |||
There was a problem hiding this comment.
good point - done
This PR extracts fetching and configuration of runtime parameters into a
separate function outside the transformers. This externalizes and
centralizes runtime parameter handling enabling, removing the
responsibility of handling runtime configuration inside transformers
(with the potential for duplication) and enabling future optimizations
of file system based caching of runtime artifacts (see #650).
This PR has a breaking change for the experimental RTV runtime URL re-writing as it now needs to be enabled explicitly via
rtv: true.