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

Implement form verification (async form validation) #9054

Merged
merged 22 commits into from May 12, 2017

Conversation

Projects
None yet
5 participants
@cvializ
Collaborator

cvializ commented Apr 29, 2017

Closes #8369

Needs documentation, opening this PR for feedback.

@cvializ cvializ requested review from jridgewell and aghassemi Apr 29, 2017

@cvializ cvializ referenced this pull request May 1, 2017

Closed

Weekly Status 2017-05-01 #9058

Show outdated Hide outdated examples/form/zipcodes.csv
@@ -0,0 +1,4431 @@
RecordNumber,Zipcode,City,State

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

What is alllllllllllllllll of this needed for?

@jridgewell

jridgewell May 1, 2017

Member

What is alllllllllllllllll of this needed for?

This comment has been minimized.

@cvializ

cvializ May 1, 2017

Collaborator

The original title of the issue was forms: zip code input validation, but it was changed. I thought reading through real California zip codes and cities would be an interesting demo. If you think it's too heavy weight we can just change it to a smaller dict.

@cvializ

cvializ May 1, 2017

Collaborator

The original title of the issue was forms: zip code input validation, but it was changed. I thought reading through real California zip codes and cities would be an interesting demo. If you think it's too heavy weight we can just change it to a smaller dict.

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

It's massive.

@jridgewell

jridgewell May 1, 2017

Member

It's massive.

Show outdated Hide outdated extensions/amp-form/0.1/form-verifiers.js
* @return {?Element}
*/
function getConfig_(form) {
return form.getElementsByTagName('script')[0];

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

This is finds deep elements. Should this be looking for a child script instead?

@jridgewell

jridgewell May 1, 2017

Member

This is finds deep elements. Should this be looking for a child script instead?

Show outdated Hide outdated extensions/amp-form/0.1/form-verifiers.js
});
const config = json && json[CONFIG_KEY];
if (!config.length) {
throw new Error(`The amp-form verification config should contain an array

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

Are there any future uses for this configuration script? Maybe we shouldn't be throwing an error if they don't specify a verification group?

@jridgewell

jridgewell May 1, 2017

Member

Are there any future uses for this configuration script? Maybe we shouldn't be throwing an error if they don't specify a verification group?

This comment has been minimized.

@cvializ

cvializ May 1, 2017

Collaborator

See #getFormVerifier. #parseConfig_ is not called if a verification group is not specified.

@cvializ

cvializ May 1, 2017

Collaborator

See #getFormVerifier. #parseConfig_ is not called if a verification group is not specified.

Show outdated Hide outdated extensions/amp-form/0.1/form-verifiers.js
throw new Error('Failed to parse amp-form config. Is it valid JSON?');
});
const config = json && json[CONFIG_KEY];
if (!config.length) {

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

This will throw for an empty JSON object.

@jridgewell

jridgewell May 1, 2017

Member

This will throw for an empty JSON object.

This comment has been minimized.

@cvializ

cvializ May 1, 2017

Collaborator

Isn't that correct behavior? If the publisher specifies {"verificationGroups": []} or {} in a form config, it seems like it is a mistake they should be made aware of through an error. Let me know if you'd prefer to allow an empty config.

@cvializ

cvializ May 1, 2017

Collaborator

Isn't that correct behavior? If the publisher specifies {"verificationGroups": []} or {} in a form config, it seems like it is a mistake they should be made aware of through an error. Let me know if you'd prefer to allow an empty config.

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

It's fine for this to be an error, but it this line causes an error. As in, the test, not the error we throw on the line below.

@jridgewell

jridgewell May 1, 2017

Member

It's fine for this to be an error, but it this line causes an error. As in, the test, not the error we throw on the line below.

* Clear the validity state of this group's elements.
*/
clearErrors() {
this.elements_.forEach(element => element.setCustomValidity(''));

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

Are all elements guaranteed to have #setCustomValidity?

@jridgewell

jridgewell May 1, 2017

Member

Are all elements guaranteed to have #setCustomValidity?

This comment has been minimized.

@cvializ

cvializ May 1, 2017

Collaborator

The HTML Constraints spec specifies that submittable elements are guaranteed to have #setCustomValidity. A publisher would have to put a non-submittable element in a verification group for this to throw, and it seems unlikely. I could check that every element in the group has a #setCustomValidity method in #parseConfig, let me know if you think that's a good idea.

@cvializ

cvializ May 1, 2017

Collaborator

The HTML Constraints spec specifies that submittable elements are guaranteed to have #setCustomValidity. A publisher would have to put a non-submittable element in a verification group for this to throw, and it seems unlikely. I could check that every element in the group has a #setCustomValidity method in #parseConfig, let me know if you think that's a good idea.

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

Yah, I would just limit them to the form input elements.

@jridgewell

jridgewell May 1, 2017

Member

Yah, I would just limit them to the form input elements.

This comment has been minimized.

@cvializ

cvializ May 4, 2017

Collaborator

Done. See #isSubmittable_

@cvializ

cvializ May 4, 2017

Collaborator

Done. See #isSubmittable_

Show outdated Hide outdated extensions/amp-form/0.1/form-verifiers.js
export class FormVerifier {
/**
* @param {!HTMLFormElement} form
* @param {!Array<!VerificationGroup>} config

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

Nit: rename to groups

@jridgewell

jridgewell May 1, 2017

Member

Nit: rename to groups

Show outdated Hide outdated extensions/amp-form/0.1/form-verifiers.js
// Set the error message on each element that caused an error.
for (let i = 0; i < errors.length; i++) {
const error = errors[i];
const element = scopedQuerySelector(this.form_, `[name="${error.name}"]`);

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

Scoped query selector is unnecessary in this case.

@jridgewell

jridgewell May 1, 2017

Member

Scoped query selector is unnecessary in this case.

This comment has been minimized.

@cvializ

cvializ May 1, 2017

Collaborator

I got an error in the presubmit on Travis telling me not to this.form_.querySelector because of strange behavior. Why is it unnecessary in this case?
If you mean use document.querySelector, multiple elements can have identical names so it could select the wrong element.

@cvializ

cvializ May 1, 2017

Collaborator

I got an error in the presubmit on Travis telling me not to this.form_.querySelector because of strange behavior. Why is it unnecessary in this case?
If you mean use document.querySelector, multiple elements can have identical names so it could select the wrong element.

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

scopedQuerySelector is only necessary when there is a space in the selector (really, when you're matching a descendent of a descendent, see #7260). In this case, you're only matching a single descendent, so no issues.

@jridgewell

jridgewell May 1, 2017

Member

scopedQuerySelector is only necessary when there is a space in the selector (really, when you're matching a descendent of a descendent, see #7260). In this case, you're only matching a single descendent, so no issues.

This comment has been minimized.

@cvializ

cvializ May 1, 2017

Collaborator

I see. We may want to update the message querySelectorAll produces during the travis presubmit or a comment in the jsdoc for scopedQuerySelector to make that clearer. Thanks!

@cvializ

cvializ May 1, 2017

Collaborator

I see. We may want to update the message querySelectorAll produces during the travis presubmit or a comment in the jsdoc for scopedQuerySelector to make that clearer. Thanks!

Show outdated Hide outdated extensions/amp-form/0.1/form-verifiers.js
// Remove the dirty flag from the successful groups that have values.
for (let i = 0; i < this.groups_.length; i++) {
const group = this.groups_[i];
if (group.isFilledOut() && group.containsNoElementOf(errorElements)) {

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

Use #shouldVerify instead.

@jridgewell

jridgewell May 1, 2017

Member

Use #shouldVerify instead.

This comment has been minimized.

@cvializ

cvializ May 1, 2017

Collaborator

This statement and the function of #shouldVerify are not equivalent. Could you please explain why?

@cvializ

cvializ May 1, 2017

Collaborator

This statement and the function of #shouldVerify are not equivalent. Could you please explain why?

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

This method is #verify, which means we're only verifying the elements that #shouldVerify returned true for.

@jridgewell

jridgewell May 1, 2017

Member

This method is #verify, which means we're only verifying the elements that #shouldVerify returned true for.

Show outdated Hide outdated extensions/amp-form/0.1/form-verifiers.js
* @param {!Array<!Element>} elements
* @return {boolean}
*/
containsNoElementOf(elements) {

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

Nit: rename to #containsNone

@jridgewell

jridgewell May 1, 2017

Member

Nit: rename to #containsNone

Show outdated Hide outdated extensions/amp-form/0.1/form-verifiers.js
*/
maybeVerify_(afterVerify) {
if (this.shouldVerify_()) {
this.doXhr_().then(() => {

This comment has been minimized.

@jridgewell

jridgewell May 1, 2017

Member

We're gonna get into a LOT of race conditions here. Need to verify that this is a response to the last issued XHR, not any before it.

@jridgewell

jridgewell May 1, 2017

Member

We're gonna get into a LOT of race conditions here. Need to verify that this is a response to the last issued XHR, not any before it.

@aghassemi

first round of comments for the first half.

@@ -57,6 +57,34 @@
color: #dc4e41;
}
.verification-form label {

This comment has been minimized.

@aghassemi

aghassemi May 2, 2017

Member

/cc @spacedino let's work with Abby on UI design of this

@aghassemi

aghassemi May 2, 2017

Member

/cc @spacedino let's work with Abby on UI design of this

This comment has been minimized.

@cvializ

cvializ May 2, 2017

Collaborator

This is just for the example page we use for development, right? It isn't baked in to amp-form. The publisher should be responsible for the spinner UX

@cvializ

cvializ May 2, 2017

Collaborator

This is just for the example page we use for development, right? It isn't baked in to amp-form. The publisher should be responsible for the spinner UX

This comment has been minimized.

@aghassemi

aghassemi May 4, 2017

Member

less so for this example page and more so to kick of the design for AmpByExample and/or AMP Start.

@aghassemi

aghassemi May 4, 2017

Member

less so for this example page and more so to kick of the design for AmpByExample and/or AMP Start.

Show outdated Hide outdated examples/forms.amp.html
},
{
"name": "fullAddress",
"elements": ["[name='addressLine2']", "[name='city']", "[name='zip']"]

This comment has been minimized.

@aghassemi

aghassemi May 2, 2017

Member

let's restrict this to just name (considering error response has name which is used to find the target elements later)

maybe: "fields" = ['addressLine2', 'city', 'zip'].

@aghassemi

aghassemi May 2, 2017

Member

let's restrict this to just name (considering error response has name which is used to find the target elements later)

maybe: "fields" = ['addressLine2', 'city', 'zip'].

this.validator_.onBlur(e);
}, true);
this.form_.addEventListener('change', e => {

This comment has been minimized.

@aghassemi

aghassemi May 2, 2017

Member

should this be guarded by custom-validation-reporting='as-you-go?

@aghassemi

aghassemi May 2, 2017

Member

should this be guarded by custom-validation-reporting='as-you-go?

This comment has been minimized.

@cvializ

cvializ May 3, 2017

Collaborator

Nope, "as-you-go" is sort of poorly named. It should be called something that implies "use your own error message elements instead of using Chrome's validation tooltip, AND do so as the user fills out fields". The change listener is still necessary when using verification with Chrome's default validation UI.

@cvializ

cvializ May 3, 2017

Collaborator

Nope, "as-you-go" is sort of poorly named. It should be called something that implies "use your own error message elements instead of using Chrome's validation tooltip, AND do so as the user fills out fields". The change listener is still necessary when using verification with Chrome's default validation UI.

This comment has been minimized.

@aghassemi

aghassemi May 4, 2017

Member

wow ok. so what is the default browser UI strategy? is it always "on-submit" and not changeable?

@aghassemi

aghassemi May 4, 2017

Member

wow ok. so what is the default browser UI strategy? is it always "on-submit" and not changeable?

This comment has been minimized.

@cvializ

cvializ May 4, 2017

Collaborator

That's right, unless you call form.reportValidation, which by default happens on submit.

@cvializ

cvializ May 4, 2017

Collaborator

That's right, unless you call form.reportValidation, which by default happens on submit.

Show outdated Hide outdated extensions/amp-form/0.1/amp-form.js
}
}
getVarSubsFields_() {

This comment has been minimized.

@aghassemi

aghassemi May 2, 2017

Member

jsdoc for this a few other private methods added

@aghassemi

aghassemi May 2, 2017

Member

jsdoc for this a few other private methods added

Show outdated Hide outdated extensions/amp-form/0.1/amp-form.js
this.setState_(FormState_.VERIFYING);
const verifyXhr = this.doVarSubs_(this.getVarSubsFields_()).then(
() => this.doXhr_(map({verify: true})));

This comment has been minimized.

@aghassemi

aghassemi May 2, 2017

Member

'verify' can collide with existing inputs named verify. Do __amp_form_verify and then check if any input has that name same way SOURCE_ORIGIN_PARAM is checked in the constructor of AMP form.

@aghassemi

aghassemi May 2, 2017

Member

'verify' can collide with existing inputs named verify. Do __amp_form_verify and then check if any input has that name same way SOURCE_ORIGIN_PARAM is checked in the constructor of AMP form.

Show outdated Hide outdated extensions/amp-form/0.1/form-verifiers.js
function parseConfig_(script) {
if (isJsonScriptTag(script)) {
const json = tryParseJson(script.textContent, () => {
throw new Error('Failed to parse amp-form config. Is it valid JSON?');

This comment has been minimized.

@aghassemi

aghassemi May 2, 2017

Member

throw user().createError() here and couple of places below.

@aghassemi

aghassemi May 2, 2017

Member

throw user().createError() here and couple of places below.

* data in ways not possible with standard form validation, or check
* values against sets of data too large to fit in browser memory
* e.g. ensuring zip codes match with cities.
* @visibleForTesting

This comment has been minimized.

@aghassemi

This comment has been minimized.

@cvializ

cvializ May 3, 2017

Collaborator

Done.

@cvializ

cvializ May 3, 2017

Collaborator

Done.

Show outdated Hide outdated extensions/amp-form/0.1/amp-form.js
this.setState_(FormState_.VERIFYING);
const verifyXhr = this.doVarSubs_(this.getVarSubsFields_()).then(
() => this.doXhr_(map({verify: true})));

This comment has been minimized.

@jridgewell

jridgewell May 2, 2017

Member

Is the map necessary?

@jridgewell

jridgewell May 2, 2017

Member

Is the map necessary?

This comment has been minimized.

@cvializ

cvializ May 3, 2017

Collaborator

Probably not. Could you help me understand what use-cases require our map utility function?

@cvializ

cvializ May 3, 2017

Collaborator

Probably not. Could you help me understand what use-cases require our map utility function?

This comment has been minimized.

@cvializ

cvializ May 3, 2017

Collaborator

I thought it was to make for-in loops more robust. I iterate over this object using a for-in loop in #doXhr

@cvializ

cvializ May 3, 2017

Collaborator

I thought it was to make for-in loops more robust. I iterate over this object using a for-in loop in #doXhr

This comment has been minimized.

@jridgewell

jridgewell May 3, 2017

Member

It has an old JS meaning and a valid CS meaning.

Old JS, people used to put enumerable properties on the Object.prototype object, which would then magically appear in all for-in loops. We don't do that. Actually we did, but Ali fixed that.

The CS meaning is when the object is treated as a true Map instance with arbitrary accesses. In that case, anything in the prototype chain could be accessed, which is incorrect in 99% of cases.

@jridgewell

jridgewell May 3, 2017

Member

It has an old JS meaning and a valid CS meaning.

Old JS, people used to put enumerable properties on the Object.prototype object, which would then magically appear in all for-in loops. We don't do that. Actually we did, but Ali fixed that.

The CS meaning is when the object is treated as a true Map instance with arbitrary accesses. In that case, anything in the prototype chain could be accessed, which is incorrect in 99% of cases.

This comment has been minimized.

@cvializ

cvializ May 3, 2017

Collaborator

I see, map is more for storing and accessing values, and we don't need it here because it's just a descriptive value and not used for storage.

@cvializ

cvializ May 3, 2017

Collaborator

I see, map is more for storing and accessing values, and we don't need it here because it's just a descriptive value and not used for storage.

Show outdated Hide outdated extensions/amp-form/0.1/amp-form.js
const isHeadOrGet = this.method_ == 'GET' || this.method_ == 'HEAD';
const p = this.doVarSubs_(varSubsFields)
.then(() => {

This comment has been minimized.

@jridgewell

jridgewell May 2, 2017

Member

Whitespace.

@jridgewell

jridgewell May 2, 2017

Member

Whitespace.

Show outdated Hide outdated extensions/amp-form/0.1/amp-form.js
return;
}
this.setState_(FormState_.VERIFYING);

This comment has been minimized.

@jridgewell

jridgewell May 2, 2017

Member

Are we going to do any blocking in this state?

@jridgewell

jridgewell May 2, 2017

Member

Are we going to do any blocking in this state?

This comment has been minimized.

@cvializ

cvializ May 3, 2017

Collaborator

Like blocking of the submit button? No, that is not planned. This state is intended to allow a spinner to show during the verify request. If the user wants to submit while the verify request is pending, they should be able to.

@cvializ

cvializ May 3, 2017

Collaborator

Like blocking of the submit button? No, that is not planned. This state is intended to allow a spinner to show during the verify request. If the user wants to submit while the verify request is pending, they should be able to.

@googlebot googlebot added the cla: yes label May 3, 2017

@aghassemi

Second round.
Excellent PR overall Carlos!

this.validator_.onBlur(e);
}, true);
this.form_.addEventListener('change', e => {
const input = dev().assertElement(e.target);

This comment has been minimized.

@aghassemi

aghassemi May 4, 2017

Member

let's create an experiment (maybe amp-form-verifier?) and guard entry points to this. Ideally being sure when experiment is off, code paths to existing forms functionality does not change (much).

@aghassemi

aghassemi May 4, 2017

Member

let's create an experiment (maybe amp-form-verifier?) and guard entry points to this. Ideally being sure when experiment is off, code paths to existing forms functionality does not change (much).

This comment has been minimized.

@cvializ

cvializ May 8, 2017

Collaborator

Done.

@cvializ

cvializ May 8, 2017

Collaborator

Done.

Show outdated Hide outdated extensions/amp-form/0.1/amp-form.js
const verifyXhr = this.doVarSubs_(this.getVarSubsFields_())
.then(() => this.doXhr_({[FORM_VERIFY_PARAM]: true}));
const reset = () => this.setState_(FormState_.INITIAL);

This comment has been minimized.

@aghassemi

aghassemi May 4, 2017

Member

what if multiple are still running and one fails while others are still running? we don't want to lose VERIFYING state.

@aghassemi

aghassemi May 4, 2017

Member

what if multiple are still running and one fails while others are still running? we don't want to lose VERIFYING state.

This comment has been minimized.

@cvializ

cvializ May 4, 2017

Collaborator

Good catch, I'll move this to the afterVerify callback

@cvializ

cvializ May 4, 2017

Collaborator

Good catch, I'll move this to the afterVerify callback

Show outdated Hide outdated extensions/amp-form/0.1/form-verifiers.js
// Set the error message on each element that caused an error.
for (let i = 0; i < errors.length; i++) {
const error = errors[i];
const element = this.form_./*OK*/querySelector(`[name="${error.name}"]`);

This comment has been minimized.

@aghassemi

aghassemi May 4, 2017

Member

scopedQuerySelector(this.form_,[name="${error.name}"]) from dom.js

@aghassemi

aghassemi May 4, 2017

Member

scopedQuerySelector(this.form_,[name="${error.name}"]) from dom.js

This comment has been minimized.

@cvializ

cvializ May 4, 2017

Collaborator

@jridgewell and I discussed this above, and he shared that scopedQuerySelector is not necessary unless there is a space in the selector. Is that your understanding as well?

@cvializ

cvializ May 4, 2017

Collaborator

@jridgewell and I discussed this above, and he shared that scopedQuerySelector is not necessary unless there is a space in the selector. Is that your understanding as well?

This comment has been minimized.

@aghassemi

aghassemi May 4, 2017

Member

My understanding is that scopedQuerySelector(parent) can match the parent element itself but parent.querySelector can never match parent. so in that sense yeah no need to use scopedQuerySelector.

With my comment I was hoping to get rid of /*OK*/ as they should be used for exceptions not common cases, looks like querySelector is allowed without `/OK/ anyway.

@aghassemi

aghassemi May 4, 2017

Member

My understanding is that scopedQuerySelector(parent) can match the parent element itself but parent.querySelector can never match parent. so in that sense yeah no need to use scopedQuerySelector.

With my comment I was hoping to get rid of /*OK*/ as they should be used for exceptions not common cases, looks like querySelector is allowed without `/OK/ anyway.

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

Neither can match the parent. The issue is #querySelector is scoped to the document, then filtered to elements inside the parent (div span can match a <div> outside parent and then a <span> inside parent). scopedQuerySelector however, is scoped to the parent only (div span can only match a <div> inside parent and a <span> inside that <div>).


#querySelector is allowed when there is a single string without any spaces (meaning, you can not have '* *'). Because this is a template string, it could do all kinds of weird things. But we know better than the linter in this case.

@jridgewell

jridgewell May 4, 2017

Member

Neither can match the parent. The issue is #querySelector is scoped to the document, then filtered to elements inside the parent (div span can match a <div> outside parent and then a <span> inside parent). scopedQuerySelector however, is scoped to the parent only (div span can only match a <div> inside parent and a <span> inside that <div>).


#querySelector is allowed when there is a single string without any spaces (meaning, you can not have '* *'). Because this is a template string, it could do all kinds of weird things. But we know better than the linter in this case.

This comment has been minimized.

@aghassemi

aghassemi May 4, 2017

Member

@jridgewell wow good to know (I probably should have known this but never too late to learn :) ). Good job preventing this with presubmit rules.

@aghassemi

aghassemi May 4, 2017

Member

@jridgewell wow good to know (I probably should have known this but never too late to learn :) ). Good job preventing this with presubmit rules.

Show outdated Hide outdated extensions/amp-form/0.1/form-verifiers.js
const error = errors[i];
const element = this.form_./*OK*/querySelector(`[name="${error.name}"]`);
if (element && element.checkValidity()) {
element.setCustomValidity(error.message);

This comment has been minimized.

@aghassemi

aghassemi May 4, 2017

Member

I guess if multiple elements have the same name (common with radio buttons) the first one gets it which is fine, let's just document here as a comment.

@aghassemi

aghassemi May 4, 2017

Member

I guess if multiple elements have the same name (common with radio buttons) the first one gets it which is fine, let's just document here as a comment.

This comment has been minimized.

@cvializ

cvializ May 4, 2017

Collaborator

Good idea. I checked how the existing validator behaves when a radio button value is required and none is checked and this matches that.
screen shot 2017-05-04 at 10 30 36 am

@cvializ

cvializ May 4, 2017

Collaborator

Good idea. I checked how the existing validator behaves when a radio button value is required and none is checked and this matches that.
screen shot 2017-05-04 at 10 30 36 am

Show outdated Hide outdated src/service/xhr-impl.js
const status = response.status;
if (status < 200 || status >= 300) {
const retriable = isRetriable(status);
const err = user().createCustomError(

This comment has been minimized.

@aghassemi

aghassemi May 4, 2017

Member

we should not log the full response (since we store logs and response body can contain sensitive info)

@aghassemi

aghassemi May 4, 2017

Member

we should not log the full response (since we store logs and response body can contain sensitive info)

This comment has been minimized.

@cvializ

cvializ May 4, 2017

Collaborator

The only thing sent in the error report is the message first argument. The other information is just tacked on to the error object and not sent, so the full response won't be logged.
Also, createError doesn't report errors so this error isn't logged anyway.

@cvializ

cvializ May 4, 2017

Collaborator

The only thing sent in the error report is the message first argument. The other information is just tacked on to the error object and not sent, so the full response won't be logged.
Also, createError doesn't report errors so this error isn't logged anyway.

This comment has been minimized.

@aghassemi

aghassemi May 4, 2017

Member

well, createError don't report but whoever is catching .fetch() error may report it. (or we might be reporting all unhandledrejection error, @jridgewell do we report unhandledrejection?)

You are right that response does got get reported right now because it is just a field on FetchError, but do we need it? I like dump error objects that one can't do anything but log them instead of error objects that can be inspected and used to switch code path. (e.g. not a fan of .catch(err => if( err.response.code == 500) {...} )

@aghassemi

aghassemi May 4, 2017

Member

well, createError don't report but whoever is catching .fetch() error may report it. (or we might be reporting all unhandledrejection error, @jridgewell do we report unhandledrejection?)

You are right that response does got get reported right now because it is just a field on FetchError, but do we need it? I like dump error objects that one can't do anything but log them instead of error objects that can be inspected and used to switch code path. (e.g. not a fan of .catch(err => if( err.response.code == 500) {...} )

This comment has been minimized.

@cvializ

cvializ May 4, 2017

Collaborator

Removing the property from the error would involve slightly more refactoring effort. The code was already augmenting the error with the response and responseJson properties, just not doing so with a custom error class.

@cvializ

cvializ May 4, 2017

Collaborator

Removing the property from the error would involve slightly more refactoring effort. The code was already augmenting the error with the response and responseJson properties, just not doing so with a custom error class.

This comment has been minimized.

@jridgewell

jridgewell May 5, 2017

Member

#7585

We do log all unhandledrejections.

@jridgewell

jridgewell May 5, 2017

Member

#7585

We do log all unhandledrejections.

Show outdated Hide outdated src/utils/promise.js
/**
* Resolves with the result of the last promise added.
*/
export class LastAddedResolver {

This comment has been minimized.

@aghassemi

aghassemi May 4, 2017

Member

awesome!

@aghassemi

aghassemi May 4, 2017

Member

awesome!

Show outdated Hide outdated src/utils/promise.js
this.count_ = 0;
if (opt_promises) {
for (let i = 0; i > opt_promises.length; i++) {

This comment has been minimized.

@aghassemi

aghassemi May 4, 2017

Member

please add a test that would catch the bug in this line.

@aghassemi

aghassemi May 4, 2017

Member

please add a test that would catch the bug in this line.

This comment has been minimized.

@cvializ

cvializ May 4, 2017

Collaborator

Ah that's a pretty goofy mistake I made. Adding the test.

@cvializ

cvializ May 4, 2017

Collaborator

Ah that's a pretty goofy mistake I made. Adding the test.

Show outdated Hide outdated extensions/amp-form/0.1/amp-form.js
inputs[i].name != SOURCE_ORIGIN_PARAM,
'Illegal input name, %s found: %s', SOURCE_ORIGIN_PARAM, inputs[i]);
const name = inputs[i].name;
user().assert(!name ||

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

First check is unnecessary.

@jridgewell

jridgewell May 4, 2017

Member

First check is unnecessary.

updatedElements.forEach(updatedElement => {
checkUserValidityAfterInteraction_(updatedElement);
});
this.validator_.onBlur(e);

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

Why are we blurring here?

@jridgewell

jridgewell May 4, 2017

Member

Why are we blurring here?

This comment has been minimized.

@cvializ

cvializ May 4, 2017

Collaborator

This function's name is not very clear. It's part of the amp-form FormValidator API and it hides any displayed messages. Originally it was always called on blur in the form, but that behavior is also needed when the verifier calls afterVerify.

@cvializ

cvializ May 4, 2017

Collaborator

This function's name is not very clear. It's part of the amp-form FormValidator API and it hides any displayed messages. Originally it was always called on blur in the form, but that behavior is also needed when the verifier calls afterVerify.

Show outdated Hide outdated extensions/amp-form/0.1/amp-form.js
const verifyXhr = this.doVarSubs_(this.getVarSubsFields_())
.then(() => this.doXhr_({[FORM_VERIFY_PARAM]: true}));
const reset = () => this.setState_(FormState_.INITIAL);

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

This probably needs to make sure form state is still VERIFYING

@jridgewell

jridgewell May 4, 2017

Member

This probably needs to make sure form state is still VERIFYING

Show outdated Hide outdated extensions/amp-form/0.1/amp-form.js
this.renderTemplate_(json || {});
this.maybeHandleRedirect_(response);
}, error => {
rethrowAsync('Failed to parse response JSON:', error);

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

This is a user error.

@jridgewell

jridgewell May 4, 2017

Member

This is a user error.

This comment has been minimized.

@cvializ

cvializ May 4, 2017

Collaborator

Thanks. Can you help me understand when something should be a user error? I haven't been able to find anything in the docs/comments.

@cvializ

cvializ May 4, 2017

Collaborator

Thanks. Can you help me understand when something should be a user error? I haven't been able to find anything in the docs/comments.

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

Developer errors are only for state that we absolutely know to be true, and isn't. So, if we expect our BaseElement instance to have an element attached, but it doesn't, that goes to dev logs.

But anything that has to do with user input, like parsing JSON that pub provides, or a response that pub provides, is on them. We don't need to know about it.

@jridgewell

jridgewell May 4, 2017

Member

Developer errors are only for state that we absolutely know to be true, and isn't. So, if we expect our BaseElement instance to have an element attached, but it doesn't, that goes to dev logs.

But anything that has to do with user input, like parsing JSON that pub provides, or a response that pub provides, is on them. We don't need to know about it.

This comment has been minimized.

@cvializ

cvializ May 4, 2017

Collaborator

It wasn't a user error before I made any changes, this is just split out from a larger function. A user error needs a tag, correct? What value should I put for tag given this is not in an amp element.

@cvializ

cvializ May 4, 2017

Collaborator

It wasn't a user error before I made any changes, this is just split out from a larger function. A user error needs a tag, correct? What value should I put for tag given this is not in an amp element.

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

That's a mistake then. This should be "FORM"

@jridgewell

jridgewell May 4, 2017

Member

That's a mistake then. This should be "FORM"

Show outdated Hide outdated extensions/amp-form/0.1/amp-form.js
this.setState_(FormState_.SUBMIT_ERROR);
this.renderTemplate_(error.responseJson || {});
this.maybeHandleRedirect_(error.response);
rethrowAsync('Form submission failed:', error);

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

User error.

@jridgewell

jridgewell May 4, 2017

Member

User error.

Show outdated Hide outdated extensions/amp-form/0.1/form-verifiers.js
// Set the error message on each element that caused an error.
for (let i = 0; i < errors.length; i++) {
const error = errors[i];
const element = this.form_./*OK*/querySelector(`[name="${error.name}"]`);

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

Neither can match the parent. The issue is #querySelector is scoped to the document, then filtered to elements inside the parent (div span can match a <div> outside parent and then a <span> inside parent). scopedQuerySelector however, is scoped to the parent only (div span can only match a <div> inside parent and a <span> inside that <div>).


#querySelector is allowed when there is a single string without any spaces (meaning, you can not have '* *'). Because this is a template string, it could do all kinds of weird things. But we know better than the linter in this case.

@jridgewell

jridgewell May 4, 2017

Member

Neither can match the parent. The issue is #querySelector is scoped to the document, then filtered to elements inside the parent (div span can match a <div> outside parent and then a <span> inside parent). scopedQuerySelector however, is scoped to the parent only (div span can only match a <div> inside parent and a <span> inside that <div>).


#querySelector is allowed when there is a single string without any spaces (meaning, you can not have '* *'). Because this is a template string, it could do all kinds of weird things. But we know better than the linter in this case.

Show outdated Hide outdated src/utils/promise.js
}, error => {
// Match the behavior of standard functions by rejecting when an error
// occurs even if it's out of order. e.g. Promise.race and Promise.all
this.reject_(error);

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

This is incorrect, we're looking for more of a cancel behavior and not a race behavior. This should just reject if the most recently added one rejects.

@jridgewell

jridgewell May 4, 2017

Member

This is incorrect, we're looking for more of a cancel behavior and not a race behavior. This should just reject if the most recently added one rejects.

This comment has been minimized.

@cvializ

cvializ May 4, 2017

Collaborator

Yeah I was on the fence about which way to go. I think it can be made to be equivalent through adding promises with a catch at the end, but I agree it's convenient to not have to do that for our common use-case.

@cvializ

cvializ May 4, 2017

Collaborator

Yeah I was on the fence about which way to go. I think it can be made to be equivalent through adding promises with a catch at the end, but I agree it's convenient to not have to do that for our common use-case.

Show outdated Hide outdated src/utils/promise.js
/**
* Resolves with the result of the last promise added.
*/
export class LastAddedResolver {

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

@ampproject/a4a will be interested in this.

@jridgewell

jridgewell May 4, 2017

Member

@ampproject/a4a will be interested in this.

Show outdated Hide outdated src/utils/promise.js
* Get the result promise.
* @return {!Promise}
*/
get() {

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

I'd be nice if this followed the thenable spec instead (we can get rid of this method then):

then(onFulfilled, onRejected) {
  return this.promise_.then(onFulfilled, onRejected);
}
@jridgewell

jridgewell May 4, 2017

Member

I'd be nice if this followed the thenable spec instead (we can get rid of this method then):

then(onFulfilled, onRejected) {
  return this.promise_.then(onFulfilled, onRejected);
}
Show outdated Hide outdated src/utils/promise.js
get() {
return this.promise_;
then(opt_resolve, opt_reject) {
if (!opt_resolve && !opt_reject) {

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

Let the promise implementation handle this.

@jridgewell

jridgewell May 4, 2017

Member

Let the promise implementation handle this.

Show outdated Hide outdated src/service/xhr-impl.js
if (status < 200 || status >= 300) {
const retriable = isRetriable(status);
const err = user().createCustomError(
new FetchError(`HTTP error ${status}`, response, retriable));

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

Why are we creating custom Error classes?

@jridgewell

jridgewell May 4, 2017

Member

Why are we creating custom Error classes?

This comment has been minimized.

@cvializ

cvializ May 4, 2017

Collaborator

I was trying to adapt the existing code to work better with the type system, since gulp check-types would complain that I was accessing properties not available on an Error instance. The previous implementation instantiated a new Error and augmented it with the response and the responseJson, in effect rejecting an error and passing data together. It was added in #3669. This felt hacky and unclear since the type was sort of lying (it says it's an Error, but it has these extra fields) so I wanted to make the augmentation more formal.

@cvializ

cvializ May 4, 2017

Collaborator

I was trying to adapt the existing code to work better with the type system, since gulp check-types would complain that I was accessing properties not available on an Error instance. The previous implementation instantiated a new Error and augmented it with the response and the responseJson, in effect rejecting an error and passing data together. It was added in #3669. This felt hacky and unclear since the type was sort of lying (it says it's an Error, but it has these extra fields) so I wanted to make the augmentation more formal.

Show outdated Hide outdated src/log.js
* @param {string} message
*/
constructor(message) {
super(message);

This comment has been minimized.

@jridgewell

jridgewell May 4, 2017

Member

Danger! This may or may not behave as you expect based on how compliant the browser is.

@jridgewell

jridgewell May 4, 2017

Member

Danger! This may or may not behave as you expect based on how compliant the browser is.

This comment has been minimized.

@cvializ

cvializ May 4, 2017

Collaborator

I ran the unit test for this on our SauceLabs suite and it passed on all platforms. Does that mitigate the danger? Do those tests cover every browser/device we support?

@cvializ

cvializ May 4, 2017

Collaborator

I ran the unit test for this on our SauceLabs suite and it passed on all platforms. Does that mitigate the danger? Do those tests cover every browser/device we support?

This comment has been minimized.

@jridgewell

jridgewell May 5, 2017

Member

I'm more worried about older versions. Extending native classes has been fraught with bugs (see https://github.com/ampproject/amphtml/blob/master/src/custom-element.js#L409-L414).

@jridgewell

jridgewell May 5, 2017

Member

I'm more worried about older versions. Extending native classes has been fraught with bugs (see https://github.com/ampproject/amphtml/blob/master/src/custom-element.js#L409-L414).

Show outdated Hide outdated extensions/amp-form/amp-form.md
@@ -361,6 +361,81 @@ The `show-all-on-submit` reporting option shows all validation errors on all inv
#### As You Go
The `as-you-go` reporting option allows your user to see validation messages as they're interacting with the input. For example, if the user types an invalid email address, the user will see the error right away. Once they correct the value, the error goes away.
## Verification

This comment has been minimized.

@aghassemi

aghassemi May 9, 2017

Member

## Verification (Experimental)

also add a first sentence that points out the experimental nature and instructions on how to enable the experiment (e.g. link to https://www.ampproject.org/docs/reference/experimental )

@aghassemi

aghassemi May 9, 2017

Member

## Verification (Experimental)

also add a first sentence that points out the experimental nature and instructions on how to enable the experiment (e.g. link to https://www.ampproject.org/docs/reference/experimental )

@cvializ

This comment has been minimized.

Show comment
Hide comment
@cvializ

cvializ May 11, 2017

Collaborator

I've tested this against some pages in the wild and our manual tests and I feel comfortable merging this now.

Collaborator

cvializ commented May 11, 2017

I've tested this against some pages in the wild and our manual tests and I feel comfortable merging this now.

@aghassemi aghassemi merged commit c244689 into ampproject:master May 12, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@aghassemi

This comment has been minimized.

Show comment
Hide comment
@aghassemi

aghassemi May 12, 2017

Member

merged

Member

aghassemi commented May 12, 2017

merged

@cvializ cvializ deleted the cvializ:cv-async branch May 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment