-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
add version to ModeDef #2918
add version to ModeDef #2918
Conversation
373a434
to
2c0bc01
Compare
@cramforce PTAL |
return { | ||
localDev: isLocalDev, | ||
// Triggers validation | ||
development: developmentQuery['development'] == '1' || window.AMP_DEV_MODE, | ||
minified: process.env.NODE_ENV == 'production', | ||
test: window.AMP_TEST, | ||
log: developmentQuery['log'], | ||
version: version, |
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.
Lets remove this. We version lock the child frame, so it can read this itself, and we need to be careful with the URL length.
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.
got it, will rename fullVersion to just version
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.
5985219
to
90c3b15
Compare
return win.AMP_CONFIG.fullVersion; | ||
} | ||
|
||
const selector = 'script[src$="/v0.js"]'; |
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.
When do we need this code path?
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.
so right now we don't have win.AMP_CONFIG.fullVersion
so this is to look for v0.js and read the version from the script tags src when the page is on the cache. I should implement #2917 by EOD. so you're right im not sure we need it. let me know what you think.
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.
removed.
added win.AMP_CONFIG.v
during build
88607b5
to
8d1cf11
Compare
@cramforce PTAL |
8d1cf11
to
107a2ea
Compare
LGTM |
Closes #2874