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

Slightly looser validation in dev-mode #20974

Closed
cramforce opened this issue Feb 20, 2019 · 14 comments

Comments

@cramforce
Copy link
Member

commented Feb 20, 2019

Allow non-AMP script tags in the JS validator if the development-mode-only attribute is present on the script tag.

This is for development environments that may want to run additional code (e.g. to do hot-reloading on code changes) while otherwise being compatible with AMP tools that do validation.

CC @timneutkens @sebastianbenz

@Gregable

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

@ampproject/wg-caching

This would be very dangerous. There may be existing AMP Caches that assume that the validator passing is sufficient to assume certain things about the document. Existing users of the AMP Validator would need to add additional validation that development-mode-only is not present on script tags. There is no way to ensure that all implementations have done so.

A few possible alternative ideas that wouldn't have this risk:

  1. A new validation mode (like we have AMP4ADS and AMP4EMAIL, etc) whose rule set is identical except for allowing arbitrary <script> tags. With this, the validator would need to explicitly opt-in to allowing this development mode.

  2. When the validator first sees a <script development-mode-only> tag, it emits an error with a special error code that is only emitted for this specific tag. Later errors for such tags are suppressed. Development environments can inspect the validator error list, rather than the PASS/FAIL value. If the error list consists of only this one error, it can choose to treat this as PASS.

I'm leaning towards 2.

Both alternatives require changes to development environments that want to support this feature, rather than changes to all production environments which don't.

@timneutkens

This comment has been minimized.

Copy link

commented Feb 20, 2019

Both alternatives require changes to development environments that want to support this feature, rather than changes to all production environments which don't.

From our standpoint we can make any needed changes in Next.js's dev environment to opt-in to the behavior 🙏

@honeybadgerdontcare

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

To generalize this to more than just script tags (in case that becomes interesting):

  • Use a new type identifier[1] development (open to bikeshedding). When the validator sees this, it emits an error message stating that the validator is operating in a developer mode which may omit errors.
  • When that type identifier is set and any tag has attribute development then that tag's errors would be suppressed.

This causes validation to still fail but is easily inspected and also extensible to any tag that needs to do something it normally wouldn't.

[1] Type identifiers sit on the html tag and causes the Validator to apply a different set of rules.

@cramforce

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2019

+1 for "technically failing validation but displaying it as passing for development mode".

I also strongly support making the dev environments do the work (of which there are zero right now), and staying on the safe side for existing users.

Generalizing to other elements might make sense. I just wanted to keep the scope controlled.

@Timer

This comment has been minimized.

Copy link

commented Jul 22, 2019

Hi there, team!

The Next.js team is getting occasional issues from users asking why their apps don't pass AMP validation in development (e.g. zeit/next.js#8050).

Is this planned for prioritization in the near future? Are there any resources the Next.js team could offer to help?

@Gregable Gregable added this to To Do in Validator: Fixit Candidates via automation Jul 24, 2019
@Gregable

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

No updates as of yet. It's currently a P3, but I've also just marked it for our next fixit sprint.

@timneutkens

This comment has been minimized.

Copy link

commented Jul 24, 2019

Awesome @Gregable 💯

@Gregable Gregable moved this from To Do to In Progress in Validator: Fixit Candidates Aug 5, 2019
@Gregable

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Fixed. You can see how this works here:
https://github.com/ampproject/amphtml/blob/master/validator/testdata/feature_tests/dev_mode.out

The requirement is that you add ampdevmode to the <html amp ampdevmode> tag, with any other attributes. This will always emit a single error on the <html> tag. The validator code for that error is DEV_MODE_ONLY and the string version is exactly:

Tag 'html' marked with attribute 'ampdevmode'. Validator will suppress errors regarding any other tag with this attribute.

This causes validation to fail, but the error code should hopefully be easy to match against and filter out downstream of the validator.

Once you do this, any other tag that you add ampdevmode to in the document will not emit any errors or warnings. This also applies to all descendants of the tag that you add ampdevmode to (with the exception of <html>), which you can use to block out errors from an entire subtree if you want to wrap some development template with a single span or something.

Errors are only suppressed. The computation from the given tag is still carried through. This means that, for example, you will not get errors about not using an included extension if you mark the usage with ampdevmode.

Feel free to reopen if this doesn't meet your needs and we can tweak as needed.

@Gregable Gregable closed this Aug 8, 2019
Validator: Fixit Candidates automation moved this from In Progress to Done Aug 8, 2019
@timneutkens

This comment has been minimized.

Copy link

commented Aug 9, 2019

Awesome @Gregable! Will have my team implement 👍

@Gregable

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

After consultation with @kristoferbaxter we will change the attribute from ampdevmode to data-ampdevmode. I'll update this issue when that is complete.

@Gregable Gregable reopened this Aug 12, 2019
Validator: Fixit Candidates automation moved this from Done to To Do Aug 12, 2019
@Gregable

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

The attribute is now data-ampdevmode. Apologies for any extra effort the churn requires.

@Gregable Gregable closed this Aug 22, 2019
@westonruter

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Will the AMP Validator extension be updated to handle this special case? Currently I see:

image

It would be useful if the icon would show a warning symbol similar to when a deprecation issue is present, or else a separate debug icon? This would be very helpful when the only validator error is just data-ampdevmode being on the html element. If there were other validation errors then indeed an error icon should still appear in the extension.

@Gregable

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

@westonruter I think the default behavior should be that this is treated as an error.

Should the extension have special casing for this? I can see the argument for it, but I also don't know that it should hide the issue. The one risk is that a publisher forgets to "fix" this issue in production, after using it to debug in test and the extension fails to alert them to that.

The "separate debug icon" seems like it could be a good recommendation. Would you file a separate issue for that suggestion?

@westonruter

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

The "separate debug icon" seems like it could be a good recommendation. Would you file a separate issue for that suggestion?

Done! See #24176.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.