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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃彈 Lint: isExperimentOn must be passed an explicit string #16688
馃彈 Lint: isExperimentOn must be passed an explicit string #16688
Conversation
|
||
module.exports = function(context) { | ||
const isExperimentOn = 'CallExpression[callee.name=isExperimentOn]'; | ||
const message = 'isExperimentOn must be passed an explicit string'; |
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.
Shouldn't message
contain instructions for the use of /*OK*/
?
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.
The only OK usages are AMP.isExperimentOn
and isExperimentOn
's own tests, because the caller themselves will have a 'amp-experiment'
string passed to it (so it's already grepable).
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.
Thanks for improving this.
Alternatively, can we extract all experiment names as shared constants to one single file. It shouldn't increase extension sizes (actually maybe even reduce the sizes), as closure compiler would be able to remove the unused ones. Linter rules might still be helpful to make sure all isExperimentOn() is called against a constant variable, but not a raw string. |
The root cause of ampproject#16671 (from ampproject#16528) is difficutly grepping for used experiment flags. So, make them very grepable. ```js // BAD const EXP = 'experiment-name'; isExperimentOn(win, EXP); // GOOD isExperimentOn(win, 'experiment-name');
1da1fa8
to
b1d401e
Compare
Codecov Report
@@ Coverage Diff @@
## master #16688 +/- ##
==========================================
+ Coverage 77.17% 78.12% +0.94%
==========================================
Files 552 553 +1
Lines 40279 40325 +46
==========================================
+ Hits 31087 31504 +417
+ Misses 9192 8821 -371
Continue to review full report at Codecov.
|
The root cause of #16671 (from #16528) is difficutly grepping for used experiment flags.
So, make them very grepable.