-
Notifications
You must be signed in to change notification settings - Fork 242
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
Fix lts #781
Fix lts #781
Conversation
The new runtime parameter handling resulted in the LTS option no longer working. Moving lts + ampRuntimeVersion in fetchRuntimeParameters. This PR also improves logging and fixes a bug where the styles fallback did not work. Fixes #777
Fallback to cdn.ampproject.org if custom runtime host cannot be resolved
@mdmower would be great if you could take a look. |
@@ -17,11 +17,12 @@ | |||
|
|||
const {AMP_CACHE_HOST, appendRuntimeVersion} = require('./AmpConstants.js'); | |||
|
|||
function calculateHost({ampUrlPrefix = AMP_CACHE_HOST, ampRuntimeVersion, lts, rtv = false}) { | |||
if (lts && ampRuntimeVersion) { | |||
throw new Error('lts flag is not compatible with runtime version parameter'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this error is worth keeping, but if (lts && ampRuntimeVersion)
should be replaced with if (lts && rtv)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Added.
`Could not download runtime version from ${ampUrlPrefix}. Falling back to ${AMP_CACHE_HOST}` | ||
); | ||
ampRuntimeData = await fetchLatestRuntimeData_({config, AMP_CACHE_HOST, lts}, versionKey); | ||
} else if (versionKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to store bad results? If not:
} else if (ampRuntimeData.version && versionKey) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mdmower - thanks for the review. PTAL
@@ -17,11 +17,12 @@ | |||
|
|||
const {AMP_CACHE_HOST, appendRuntimeVersion} = require('./AmpConstants.js'); | |||
|
|||
function calculateHost({ampUrlPrefix = AMP_CACHE_HOST, ampRuntimeVersion, lts, rtv = false}) { | |||
if (lts && ampRuntimeVersion) { | |||
throw new Error('lts flag is not compatible with runtime version parameter'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Added.
`Could not download runtime version from ${ampUrlPrefix}. Falling back to ${AMP_CACHE_HOST}` | ||
); | ||
ampRuntimeData = await fetchLatestRuntimeData_({config, AMP_CACHE_HOST, lts}, versionKey); | ||
} else if (versionKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
The new runtime parameter handling resulted in the LTS option no longer
working. Moving lts + ampRuntimeVersion in fetchRuntimeParameters.
This PR also improves logging and fixes a bug where the styles fallback
did not work.