Add a feature that checks arguments to 3p embeds #1211
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| continue; | ||
| } | ||
| if (allowedFields.indexOf(field) == -1) { | ||
| assert(false, 'Unknown attribute for %s: %s.', data.type, field); |
There was a problem hiding this comment.
Couldn't the if condition be done in the assert call?
There was a problem hiding this comment.
Yes, good catch. I was refactoring this and now it is possible.
|
LGTM, besides the two comments. |
| field in defaults) { | ||
| continue; | ||
| } | ||
| assert(allowedFields.indexOf(field) == -1, |
There was a problem hiding this comment.
Was just testing the tests :)
| * @param {!Array<string>} allowedFields | ||
| */ | ||
| export function validateData(data, allowedFields) { | ||
| const defaults = { |
There was a problem hiding this comment.
can we move this to the outer scope?
There was a problem hiding this comment.
Do you feel strongly?
I renamed it to make the purpose more straight forward, but I did not move it.
- in practice this function is only called once per compilation unit (so there is no double allocation)
- it is harder to read when the constant is far away.
| @@ -29,7 +29,7 @@ import {adsense} from '../ads/adsense'; | |||
| import {adtech} from '../ads/adtech'; | |||
| import {doubleclick} from '../ads/doubleclick'; | |||
| import {twitter} from './twitter'; | |||
| import {register, run} from '../src/3p'; | |||
| import {register, run, checkData} from '../src/3p'; | |||
There was a problem hiding this comment.
Yeah, we really need a lint check for this.
There was a problem hiding this comment.
we can turn it on, but right now it will complain about (if i remember correctly):
class SomeClass {
constructor() {
/** @private */
this.someProp_;
}
}
cramforce
force-pushed
the
3p-args
branch
4 times, most recently
from
07ab34d
to
4fdff92
Compare
| }); | ||
| }); | ||
| import {validateSrcPrefix, validateSrcContains, checkData, validateData} | ||
| from '../../src/3p'; |
…are being sent from the parent frame. Previously we would just silently ignore these, which leads to very hard to debug errors. Especially since these are often optional. The new errors do not interrupt execution to avoid each deprecation being a breaking change. Unrelated to the rest of the PR: makes the `amp-iframe` more robust against flakes.
cramforce
added a commit
that referenced
this pull request
Dec 21, 2015
Add a feature that checks arguments to 3p embeds
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
… and throws if an unexpected argument appears.
Previously we would just silently ignore these,
which leads to very hard to debug errors. Especially since these are often
optional. The new errors do not interrupt execution to avoid each deprecation
being a breaking change.
Unrelated to the rest of the PR: makes the
amp-iframemore robust against flakes.