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

Prevent output of scripts and styles in AMP to avoid validation errors #332

Closed

Conversation

@westonruter
Copy link
Contributor

commented Apr 24, 2018

Version 0.7 of the AMP plugin adds amp theme support. When present, any erroneous scripts or styles which are invalid AMP are detected and reported as validation errors. This PR short-circuits Query Monitor from outputting any scripts or styles when serving AMP. Eventually it would be nice if there was a lightweight AMP display of the QM data, but this at least makes it so that Query Monitor doesn't interfere with AMP responses.

@johnbillion

This comment has been minimized.

Copy link
Owner

commented Apr 24, 2018

Thanks for the PR.

In this regard, is QM different from any other plugin which enqueues JS on the front end?

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2018

No, it's not different. It's just by explicitly skipping the enqueueing of scripts and styles, the AMP plugin then doesn't have to warn about their removal. Since QM doesn't work in AMP anyway, it seems better to just short-circuit it to begin with.

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2018

I guess it is different then. The difference is that where another plugin may enqueue CSS on the frontend and expect that it should work in AMP, since QM doesn't work in AMP at all then the assests make sense to not be enqueue to begin with.

@johnbillion

This comment has been minimized.

Copy link
Owner

commented Apr 25, 2018

Cheers. I'm still trying to figure out how this is different from any other plugin enqueuing CSS or JS on the front end that AMP determines is invalid.

For example, if I build a plugin that adds a tracking script to the page and AMP determines that it's invalid, does that plugin need to add conditional logic so as to not load it when AMP is in use?

It seems that the onus should be on AMP to not enqueue assets that it determines aren't valid, otherwise every plugin there is potentially needs to add conditional logic for AMP.

Am I understanding correctly?

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

You're right, it's technically no different in that way from any other plugin. The AMP plugin won't let any scripts get output even if they are enqueued. It removes any scripts that are printed, but as it does so it reports that as a validation error. Since a site's functionality may depend on a given script to be present, it is not safe for the plugin to just remove scripts at will. So the plugin will soon actually auto-disable AMP for a given URL when such scripts are present; it will require a user to explicitly authorize those scripts to be removed from the page, and in doing so re-enable AMP.

So by skipping the printing of scripts, Query Monitor would bypass requiring a user to go through this step of going through the validation errors. Since a site's functionality doesn't depend on QM outputting these scripts, it would be nice if QM could acknowledge this by not enqueueing them when is_amp_endpoint(). On the other hand, if a plugin had some tracking script output then this isn't something that the plugin should just skip outputting and another solution would be needed.

Does this make sense?

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2018

On thinking about this some more, I am going to withdraw my PR from consideration as it is essentially a cop out. The right thing to do would be to add a new dispatcher specifically for AMP responses, just as there are dispatchers for REST and Ajax. Would you be open to see a PR for that? I see there as being an interesting way that QM could display some information that currently is limited to Server-Timing headers in the AMP plugin, to list out the time required to run the various sanitizers/processors to serve content as AMP. Additionally, Query Monitor could be used to display the AMP validation errors instead of requiring users to go to an admin screen.

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

@johnbillion Circling back on this now, the AMP plugin now supports a Dev Mode which allows the Admin Bar to be included in the page while maintaining AMP validity. I've got a full writeup about it: https://weston.ruter.net/2019/09/24/integrating-with-amp-dev-mode-in-wordpress/

Here's a plugin which shows how Query Monitor can be made AMP-compatible: https://gist.github.com/westonruter/621137b5a5ae1caaaee48c63f61ce7b7

You can see that it just involves adding data-ampdevmode attributes to the required elements. Is this something you'd be open to adding directly to Query Monitor?

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