Skip to content
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 a feature that checks arguments to 3p embeds #1211

Merged
merged 1 commit into from
Dec 21, 2015

Conversation

cramforce
Copy link
Member

… 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-iframe more robust against flakes.

continue;
}
if (allowedFields.indexOf(field) == -1) {
assert(false, 'Unknown attribute for %s: %s.', data.type, field);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't the if condition be done in the assert call?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good catch. I was refactoring this and now it is possible.

@jridgewell
Copy link
Contributor

LGTM, besides the two comments.

field in defaults) {
continue;
}
assert(allowedFields.indexOf(field) == -1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!= -1, I believe.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was just testing the tests :)

* @param {!Array<string>} allowedFields
*/
export function validateData(data, allowedFields) {
const defaults = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move this to the outer scope?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm. 👍

@@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we really need a lint check for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can turn it on, but right now it will complain about (if i remember correctly):

class SomeClass {
  constructor() {
    /** @private */
    this.someProp_;
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@cramforce cramforce force-pushed the 3p-args branch 4 times, most recently from 07ab34d to 4fdff92 Compare December 21, 2015 22:02
});
});
import {validateSrcPrefix, validateSrcContains, checkData, validateData}
from '../../src/3p';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent 2 more spaces.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

…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
@cramforce cramforce merged commit 4128daf into ampproject:master Dec 21, 2015
@cramforce cramforce deleted the 3p-args branch December 21, 2015 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants