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.

Sign up for GitHub

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

Jump to bottom

Elaborate error message for amp-script with attribute data-ampdevmode #29457

Open
morsssss opened this issue Jul 23, 2020 · 27 comments
Open

Elaborate error message for amp-script with attribute data-ampdevmode #29457

morsssss opened this issue Jul 23, 2020 · 27 comments
Assignees
@samouri
Labels
P3: When Possible Type: Bug WG: caching

Comments

@morsssss
Copy link
Contributor

In #27076, we made it legal to use data-ampdevmode on an <amp-script> component even if the attribute was not included on the root html node. An example:

<html ⚡ lang="en">
...
<amp-script layout="fixed" height="300" width="200" data-ampdevmode>
...
</html>

This approach works! However, it throws a validation error in the Chrome extension, even though it doesn't throw one in the Console:

The attribute 'data-ampdevmode' in tag 'amp-script' is set to the invalid value ''.

image
@morsssss
Copy link
Contributor Author

/cc @samouri

@samouri samouri self-assigned this Jul 31, 2020
@samouri
Copy link
Member

samouri commented Jul 31, 2020 •
edited
Loading

This seems like a bug with the extension itself. I'll track this down (or at least get in contact with the right people to track it down)

@dreamofabear dreamofabear mentioned this issue Aug 3, 2020
Closed
@dreamofabear dreamofabear mentioned this issue Aug 17, 2020
Closed
@honeybadgerdontcare
Copy link
Contributor

@morsssss Are you sure it's not throwing one in console? If so, could you message me the URL?

I'm getting this error on http://validator.amp.dev/ as well, so it's not just an extension issue.

The source says it should always cause an error for data-amp-devmode on amp-script.

@morsssss
Copy link
Contributor Author

Hey @honeybadgerdontcare !

Well, I get the proper warning in the Console. Here's a screenshot:

image

If you like I can stick a sample up on codepen or the like. But I get it whenever I use data-ampdevmode on the <amp-script> but not on the <html> tag.

I agree that it this should always cause a validation error or warning... the one shown in the Console is perfect for me.

@honeybadgerdontcare
Copy link
Contributor

Hello @morsssss !

That message you're seeing in the console is not a validator error or warning. It is given by the AMP extension script, see

amphtml/extensions/amp-script/0.1/amp-script.js

Line 137 in 196f517

if (this.development_) {
.

The validator only displays warnings and errors to the console when #development=1 is attached to the URL. Are you not seeing the one given at validator.amp.dev in the console when #development=1 is attached to the URL?

@morsssss
Copy link
Contributor Author

Yes... oh, I see. I don't think I understood what you were asking.

Yes, of course, if I add #development=1, I see validation errors in the Console, as always.

(Is that what you were asking?)

You can ignore my comment earlier about not seeing an error in the Console... throughout my testing of amp-script, I've found that the extension or WorkerDOM have thrown uncaught errors, so that's what I was referring to. Nothing about the validator!

@honeybadgerdontcare
Copy link
Contributor

Yes, that is what I was asking. So this is working as intended. The console shows validator errors in console with #development=1 and the webui and extension also show the same error.

@morsssss morsssss reopened this Aug 26, 2020
@morsssss
Copy link
Contributor Author

The point of this issue is to change that error message. It's not correct. Using data-ampdevmode is normal, and the attribute doesn't require a value. It's just that we don't want documents that use this attribute to pass validation.

If the validation error message were changed to something that resembles the warning thrown by the script, we'd be all set.

@honeybadgerdontcare
Copy link
Contributor

Updating subject to be more clear.

Request is to change error message for specific case of amp-script having attribute data-ampdevmode on it to be JavaScript size and script hash requirements are disabled in development mode..

It should also probably be clear that the page is still invalid AMP with that attribute.

@honeybadgerdontcare honeybadgerdontcare changed the title Incorrect data-ampdevmode validation error in extension only Elaborate error message for amp-script with attribute data-ampdevmode Aug 26, 2020
@morsssss
Copy link
Contributor Author

Perfect - thanks!

@samouri samouri mentioned this issue Aug 28, 2020
Closed
@samouri
Copy link
Member

samouri commented Sep 14, 2020

@honeybadgerdontcare: just to confirm, is this an issue that wg-caching will take care of or should I try to prioritize it for wg-runtime?

@honeybadgerdontcare
Copy link
Contributor

@samouri wg-caching is willing to review PRs from anyone =D, however note that this would be a string used internally at Google and needs to be internationalized. so a PR isn't sufficient for it as there would also need to be additional internal changes for it.

for wg-caching, this is a p3

@morsssss
Copy link
Contributor Author

For <amp-script>, I regard this as more than a P3. data-ampdevmode should be used by almost all developers who want to write custom JS in AMP. But this validation error message makes it sound like the developer's using it incorrectly. It's bound to be confusing many developers.

Is there a path to raising the priority?

@honeybadgerdontcare
Copy link
Contributor

I really do not see this as being a higher priority than a p3. It's adding nuance to an existing error message. Developers adding this attribute know that it shouldn't be on the final document as it's a developer tool.

@morsssss
Copy link
Contributor Author

morsssss commented Sep 14, 2020 •
edited
Loading

+ @kristoferbaxter , + @choumx to get another opinion

@morsssss
Copy link
Contributor Author

or + @patrickkettner ?

@dreamofabear
Copy link

Validator errors typically don't have extension-specific language. What if we added nuance to the console warning?

"JavaScript size and script hash requirements are disabled in development mode (development mode is invalid AMP)."

@Gregable
Copy link
Member

Gregable commented Sep 21, 2020 •
edited
Loading

I think we've backed ourselves into a bad state here, one that is confusing beyond just the error message. After reviewing this a little bit, I'm not super happy that we have two very divergent meanings for data-ampdevmode depending on context.

The first meaning is a validation suppression one. Specifically the semantics are:

By default, just like any other data- attribute, data-ampdevmode is allowed on any element in the DOM and will not emit warnings/errors. If data-ampdevmode is added to the <html> element, as a special case, it will emit a single very clear error invalidating the document. Additionally it will enable a new validator error reporting option for the validator for all child elements of the <html>. Specifically, if a child element also has data-ampdevmode added, no validator errors or warnings will be emitted for that child element or any of it's descendants. The attribute is not intended to have any impact on runtime behavior regardless of location, it's validator specific.

The second meaning was recently added and apparently is specific to runtime. Specifically the semantics are:

If and only if the attached element is <amp-script>, this is also a validation error. If attached to <amp-script>, there is some runtime disabling of some verifications on amp-script's behavior.

The issues include:

  1. There is some confusion over the subtree validator supression semantics. This is something we need better documentation for, as I think the original github issue is the only place where this behavior is clearly described. Wherever we place this documentation, it should be linked to by the validator error messages. Filed Missing documentation on data-ampdevmode amp.dev#4671 for this.
  2. ampdevmode is perhaps too vague of a name for validator suppression. There were suggestions to improve the name offered in amp-script: Decouple disabling script hash from data-ampdevmode #26341. I think @westonruter explains that we may be too late to change the name here given existing usage, so I'm not inclined to make changes for the validator supression naming.
  3. The semantics of enabling the dev mode with the html tag differ in the specific case of the amp-script tag. The idea of the "mode" in validation suppression is that it's a document-level flag that allows support for subtree supression. The idea of the "mode" in the amp-script tag is that it's a tag-level flag for amp-script.

I think #3 above needs to be solved beyond improving the error message. Specifically, I would like to propose we implement one of the following options:

  1. Trigger the <amp-script> runtime error suppression behavior if and only if data-ampdevmode is set in the <html> tag of the document, in other words trigger on the global document-level flag semantics. Revert the special case validator error added in amp-script: decouple dev modes #27076.
  2. Trigger the <amp-script> runtime error suppression behavior if and only if data-ampdevmode is set in the <html> tag of the document AND the <amp-script> tag, in other words trigger on the global document-level flag semantics combined with the subtree suppression semantics. Revert the special case validator error added in amp-script: decouple dev modes #27076.
  3. Trigger the <amp-script> runtime error suppression behavior based on a new attribute distinct from data-ampdevmode. Add validator code to emit a specific actionable error when this attribute appears.

@morsssss
Copy link
Contributor Author

My concern is, if I'm new to amp-script (as most AMP developers still are), and I see this error in my validator:

The attribute 'data-ampdevmode' in tag 'amp-script' is set to the invalid value ''.

I will assume that I've used this attribute in a way that's illegal. In other words, I'll think I should be setting data-ampdevmode to some other value. I won't know that, actually, the validator's telling me that this is invalid because it's only to be used in development, not in production. I think this presents a substantial obstacle to developers who'd like to start using amp-script.

I see in the code that this string isn't specific to a certain extension, as you point out, @choumx !

But if we can indeed append an extension-specific string, I'd suggest the final message read something like this:

The attribute 'data-ampdevmode' in tag 'amp-script' is set to the invalid value ''. However, this can be used in development to suppress JavaScript size and script hash requirements.

This isn't perfect but I think it's an improvement.

@morsssss
Copy link
Contributor Author

And, @Gregable , looks like we were commenting at the same time!

I was involved in the discussion in #27076, but I agree that the current state has become confusing. I'm fundamentally happy with any solution that results in the action in your option #3 above:

Add validator code to emit a specific actionable error when this attribute appears.

@Gregable
Copy link
Member

@samouri @choumx to determine next steps for amp-script in runtime.

@Gregable Gregable mentioned this issue Sep 23, 2020
Closed
@dreamofabear
Copy link

Hehe, option (3) was actually the original state -- amp-script[development] was the invalid attribute that skipped the hash check, which was later merged into data-ampdevmode. More context in #24987.

I suppose the original source of troubles was the ambiguous reuse of "dev/development". We could just:

  1. Remove the amp-script special-case for data-ampdevmode
  2. Add a new, invalid amp-script attribute e.g. skip-hash.

A long detour for renaming an attribute but lessons were learned. :)

@Gregable
Copy link
Member

@choumx Does that mean you prefer option (3)? We can do a little more here and provide a very specific message for this invalid attribute, if useful.

@dreamofabear
Copy link

Yea, it seems like the best option.

@morsssss
Copy link
Contributor Author

thanks, everyone!

@samouri
Copy link
Member

samouri commented Feb 2, 2021 •
edited
Loading

Add a new, invalid amp-script attribute e.g. skip-hash.

The naming isn't that clear. The current dev mode doesn't just skip-hash, but also skips size checks. will try to brainstorm another attribute name

@stale
Copy link

stale bot commented Jul 30, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Inactive for one year or more label Jul 30, 2022
@Gregable Gregable removed their assignment May 30, 2023
@stale stale bot removed the Stale Inactive for one year or more label May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@samouri samouri

Labels
P3: When Possible Type: Bug WG: caching
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@Gregable @honeybadgerdontcare @samouri @banaag @dreamofabear @morsssss