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

Update versioning/deprecations policy #17370

Merged
merged 12 commits into from Aug 17, 2018
Merged

Conversation

dreamofabear
Copy link

@dreamofabear dreamofabear commented Aug 8, 2018

Context: #16691 (comment)

Changes in this PR

  • Modify goal: "Keep all users of AMP on secure and updated versions of the library."
  • Add goal: "Enable all AMP developers to keep their AMP pages up-to-date."
  • Make minor version useful: "Minor versions may be changed for breaking behavior changes without breaking API changes."
  • Adds "version deprecations" subsection with deprecation timelines.
  • Minor reorganizing and clean up.

Other change proposals

Search Console warning

Current:

Search Console has identified that your site is affected by N new AMP related issues. This means that AMP may be negatively affected in Google Search results.

Suggested:

Search Console has identified that your site is affected by N new AMP-related warnings. Warnings do not impact AMP in Google Search results. However, some warnings will eventually become errors. Errors may negatively affect AMP in Google Search results.

Validator warning

Current:

The extension 'amp-mustache' is referenced at version '0.1' which is a deprecated version. Please use a more recent version of this extension. This may become an error in the future.

Suggested:

The extension 'amp-mustache' is referenced at version '0.1' which was deprecated on 2018-01-30. This version will be supported until it enters end-of-life on 2019-01-30, after which this warning may become an error. Please update to a more recent version of this extension.

- If a change is backward compatible, no version should be changed.
- This means that, for now, there is no known case under which the minor version of an extension would be changed.
- Minor versions may be changed for breaking behavior changes *without* breaking API changes.
- Major versions *must* be changed for any breaking API changes.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cramforce Alternative: Remove useless minor version from extensions moving forward.

- Changes to CSS included in AMP that are not backward compatible.
- Changes to the DOM structure (elements, attributes and attribute values) generated by AMP extensions that are not backward compatible.

The following special cases are not considered breaking changes:

- Changes to elements and their children including their CSS if the element name starts with `i-`.
- Changes to attribute and class names starting with `-`. But this doesn't mean, CSS backward compatibility may be broken with this mechanism, where it would otherwise be considered a breaking change.
- Changes to attribute and class names starting with `i-` (but this doesn't mean that CSS backward compatibility may be broken with this mechanism, which would be considered a breaking change).
- Changes required to maintain the security of AMP pages.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security changes can be breaking by the definition above. I think this should come out of here and a note added that says something to the effect of "Changes required to maintain the security of AMP pages may not follow this policy if doing so would be detrimental to security"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the security point to its own subsection.


- Changes to the semantics of supported attributes and other exposed APIs.
- Changes to exposed API semantics, e.g. supported attributes in an AMP extension.
- Changes to CSS included in AMP that are not backward compatible.
- Changes to the DOM structure (elements, attributes and attribute values) generated by AMP extensions that are not backward compatible.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Call out changes to what is considered a valid document?


## Breaking changes

Breaking changes are:
Breaking changes must be implemented in a new version. Breaking changes are defined as:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new version of amp or new version of extension?

How would this rule apply to a change to validation of non-extension html? For a concrete example, I'm trying to understand how these rules would apply to changing the syntax of the AMP boilerplate css.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new version of amp or new version of extension?

Made this clearer.

How would this rule apply to a change to validation of non-extension html?

I think boilerplate CSS would fall under the new validation breaking change line item.

- Changes required to maintain the security of AMP pages.
- Changes that break fewer than 0.1% of crawler accessible AMP pages.
- Changes that break fewer than 0.1% of crawler-accessible AMP pages.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'break' / 'breaking' isn't well defined here. It's easy for questions of validation.

How would this apply to a behavioral change? For a contrived example, if all amp documents were suddenly modified so that the default text color was green. Or if all images were automatically given alt tags?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. This policy doesn't yet consider "enhancing" Cache transformations e.g. srcset writing and blurry image placeholders. I'll add some language to accommodate that.


## Deprecation process
## Deprecations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do deprecations that are breaking interact with the rules for breaking changes above?

If the deprecation process is followed to introduce a breaking change, are the same requirements still at play (security -or- < 0.1%), or does the deprecation process represent an alternative route / exception to making breaking changes?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latter. Made this clearer, thanks.


The process is
In rare cases, the AMP project may decide that an existing feature or API must be removed. Such breaking changes must follow the AMP deprecations process prior to removal. Deprecations must be publicly discussed and provide significant user benefit that justifies additional work for page developers.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please clarify that features should never be deprecated before the replacement has been available for at least N weeks (N should be bigger than 2)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The subsection below spells this out:

  • A version must not be deprecated until a new version is released and stable for at least 1 month.
  • A version must not be invalidated until it has been deprecated for at least 1 year.

@dreamofabear dreamofabear merged commit af504f3 into master Aug 17, 2018
@dreamofabear dreamofabear deleted the deprecations-proposal branch August 17, 2018 18:39
Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
* Update amp-versioning-policy.md

* Minor edits

* Add some links, tweak language.

* Tweak goals.

* s/may/must, do not allow removal of binaries.

* Remove semver reference.

* Clarify breaking changes, version removal.

* Make security breaking changes clearer.

* Validation can be breaking, relate to deprecations

* Clean up version deprecations language.

* Move supscript position.

* Missing a word, clarification.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants