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

Discovery: Optional/experimental transformers #4213

Closed
kmyram opened this issue Feb 3, 2020 · 7 comments
Closed

Discovery: Optional/experimental transformers #4213

kmyram opened this issue Feb 3, 2020 · 7 comments
Assignees
Labels
Changelogged Whether the issue/PR has been added to release notes. Discussion For issues that are high-level and not yet ready to implement. WS:Perf Work stream for Metrics, Performance and Optimizer
Milestone

Comments

@kmyram
Copy link

kmyram commented Feb 3, 2020

Feature description

Identifying and determining issues from non-standard transformers when establishing feature parity between Go and PHP versions of optimizer libs.

Transformers differ in scope across versions with local variance. In Go, some transformers have split up same functionality over multiple tasks providing the same functionality in the end but with different process. The goal is parity of functionality but providing this with low complexity and a robust framework for PHP.

Derived from #958
Relates to #4214, #4215, #4216


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • Listing optional/experimental transformers
  • Determine importance/priority of each of these
  • Provide brief description of possible implementation in PHP

Implementation brief

  • N/A

QA testing instructions

  • N/A

Demo

  • N/A

Changelog entry

  • First draft
@kmyram kmyram added the Discussion For issues that are high-level and not yet ready to implement. label Feb 3, 2020
@kmyram kmyram added this to the v1.5 milestone Feb 3, 2020
@westonruter westonruter modified the milestones: v1.5, v1.6 Mar 17, 2020
@westonruter
Copy link
Member

I've filed one issue related to this, but it's for a transformer that doesn't exist yet: #4439

@schlessera schlessera self-assigned this Mar 26, 2020
@schlessera
Copy link
Collaborator

@schlessera
Copy link
Collaborator

schlessera commented Mar 27, 2020

Conclusion

[Extract from the above document]

My recommendation is based on the following assumptions:

  • The optimizer should be optional for generating valid AMP. This entails that changes that are required for valid AMP will have to go into the Sanitizer instead.
  • We want to focus on new functionality and deprioritize transformers that would provide functionality that is currently already covered by logic in the plugin itself.
  • We want to prioritize transformers that have a noticeable impact on frontend performance on a typical WordPress site.

Given these assumptions, here is a numbered list of tasks to include for the next release, sorted by priority:

  1. ServerSideRendering transformer with the following functionality:
  2. Minify transformer with the following functionality:
  3. BrowserHints transformer with the following functionality:
  4. BlurredPlaceholders transformer

All other transformers/functionalities are either high-cost/low-benefit or already provided by the plugin and only needed for 2.0 (assuming that’s when we split out the optimizer library into a separate package).

cc/ @westonruter, @amedina

@westonruter
Copy link
Member

I agree with your prioritization.

  • The optimizer should be optional for generating valid AMP. This entails that changes that are required for valid AMP will have to go into the Sanitizer instead.

Perhaps we should divide the Optimizer's transformers into two sets: required and optional. The required ones we can always invoke to avoid having to duplicate the logic in the sanitizers. But given that the Optimizer is an optimizer, are there any transformers used for generating valid AMP? Yes, I suppose transformers like AutoExtensionImporter, AddMandatoryTags, and AmpScriptCsp.

As part of the modularization effort, we should be migrating the existing sanitizers into a new standalone PHP package. Should the scope of Optimizer be increased to be an AMP Processor? One class of transformers would be for optimization, another would be for conversion (e.g. img to amp-img) and another for sanitization/validation. This seems to be where the AMP Toolbox Optimizer is going as well. Note that the Optimizer has already moved into this territory already by implementing a Markdown transformer. I can imagine the AMP Toolbox Optimizer would at some point be able to take an arbitrary HTML page and do similar conversions/sanitizations that the AMP plugin is doing as well. Perhaps this would be a key area to collaborate on a common spec for how such auto-conversions are done (and also for oEmbed handling) and then we can have a Node.js implementation and a PHP implementation.

/cc @sebastianbenz

@westonruter
Copy link
Member

All this to say, then the PHP Optimizer AMP Processor would no longer be optional. It would be required, but some of the transformers that purely handle optimization would be optional.

@sebastianbenz
Copy link

Agree with @westonruter. Having a common spec for transformers (with an easy to use test suite) would be a good thing to have.

@kmyram kmyram removed the Size: 3 label Apr 9, 2020
@westonruter
Copy link
Member

Closing as I believe the discovery has been completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. Discussion For issues that are high-level and not yet ready to implement. WS:Perf Work stream for Metrics, Performance and Optimizer
Projects
None yet
Development

No branches or pull requests

4 participants