✨Created the initial <amp-recaptcha-input> 3p Bootstrap Iframe #17747
Conversation
|
@zhouyx This is now good to go for review 😄 |
3p/recaptcha.js
Outdated
| * @param {function(*)} cb | ||
| */ | ||
| function getRecaptchaApiJs(global, scriptSource, cb) { | ||
| writeScript(global, scriptSource, function() { |
There was a problem hiding this comment.
Can we load the script asynchronously with loadScript method?
There was a problem hiding this comment.
Completely up to your preference, but I kind of feel it's unnecessary to have the getRecaptchaApiJs method as it is only a wrapper to the writeScript, doesn't do anything extra
| * @param {!Window} global | ||
| * @param {!Object} data | ||
| */ | ||
| export function recaptcha(global, data) { |
There was a problem hiding this comment.
nit: Can we move this function to the top? So that I read the most important function at first : ) It's easier for code review. Thank you
|
|
||
| grecaptcha.execute(siteKey, { | ||
| action: data.action, | ||
| })./*OK*/then(function(token) { |
There was a problem hiding this comment.
I feel we always use token => {...} in the code base. Not sure if it is a code style thing : ) If so maybe it's worth switching.
There was a problem hiding this comment.
So the reason I haven't been using phat arrow functions is because I noticed the other 3p scripts use the normal function callbacks. I think the reasoning is that the other 3p scripts may be from when we still supported IE11, which does not support the phat arrow ES6 syntax. And since I am assuming we don't have a phat arrow polyfill for the 3p scripts, I think we should stick with this style. 😄 Let me know what you think.
There was a problem hiding this comment.
Sounds good! Thanks for the detailed explanation.
3p/recaptcha.js
Outdated
| ); | ||
|
|
||
| let recaptchaApiUrl = 'https://www.google.com/recaptcha/api.js?render='; | ||
| const siteKey = data.sitekey || '6LebBGoUAAAAAHbj1oeZMBU_rze_CutlbyzpH8VE'; |
3p/recaptcha.js
Outdated
| 'action', | ||
| actionTypeHandler.bind(this, grecaptcha, siteKey) | ||
| ); | ||
| iframeMessagingClient./*OK*/sendMessage('ready'); |
There was a problem hiding this comment.
In general I think it's better to add prefix to all messages here. So we never mess up with same names. What about amp-recaptcha-ready?
3p/recaptcha.js
Outdated
| */ | ||
| function actionTypeHandler(grecaptcha, siteKey, data) { | ||
| if (!iframeMessagingClient) { | ||
| user().error(TAG, 'IframeMessagingClient does not exist.'); |
There was a problem hiding this comment.
Sounds like a dev error to me instead
|
@zhouyx This is good to go for another review 😄 |
|
|
||
| grecaptcha.execute(siteKey, { | ||
| action: data.action, | ||
| })./*OK*/then(function(token) { |
There was a problem hiding this comment.
nit: add comment that the promise is polyfilled.
3p/recaptcha.js
Outdated
| }).catch(function(err) { | ||
| user().error(TAG, err); | ||
| iframeMessagingClient./*OK*/sendMessage(MESSAGE_TAG + 'error', dict({ | ||
| 'error': (err || '').toString(), |
There was a problem hiding this comment.
should we use error.message? Or error.name? What exactly are we expecting here?
|
LGTM! |
…oject#17747) * Started the recaptcha 3p frame * Got the recaptcha api script loaded and working * Finished initial implementation of 3p recaptcha * Handled errors and used existing iframe APIs * MOved to the iframe messaging client * Fixed other errors, simply need to handle presubmit * Fixed other presubmit errors, need to refactor iframe messaging * Fixed all issues with builds and implementation * Made requested PR changes * Made some more PR fixes
relates to #2273
This creates the initial 3p Bootstrap Iframe for
<amp-recaptcha-input>. Here are some things to note:recaptcha-bootstrap.jsto justrecaptcha.jsto be consistent, and work withintegration.jseasier.IframeMessagingClientfor messaging, instead ofmessaging.jsI tested this using a gitignored modified
amp-gistcomponent. Also, it is worth noting, this has no tests, as it seems all the 3p API type services don't have tests, only the iframe messaging systems do.Example
This example was made with a modified version of this PR, and I modified
<amp-gist>just to show the recaptcha 3p bootstrap iframe and it works! 😄