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

Deprecating cdn.ampproject.org/v0/validator.js in favor of WebAssembly version #36110

Closed
honeybadgerdontcare opened this issue Sep 20, 2021 · 22 comments
Assignees
Labels
INTENT TO DEPRECATE Proposes deprecating an existing AMP feature. P2: Soon WG: caching

Comments

@honeybadgerdontcare
Copy link
Contributor

Summary

The AMPHTML Validator ensures that critical features of the AMPHTML spec are followed within AMP related documents. It is maintained by a set of rules and engines written in C++ and JavaScript that apply those rules during validation. There are many developer tools that utilize these validator engines to assist in the development of valid AMP documents.

Due to ongoing complexity and developer maintenance of two validator engines, the Caching Working Group is deprecating the native JavaScript validator in favor of a WebAssembly validator which uses the C++ engine.

Existing developer tools such as the Validator Web UI, Chrome Extension, VS Code Extension and Node.js Package have already migrated to the WebAssembly validator.

To inform existing developers using validator.js directly and not through one of these developer tools, the APIs in validator.js will be deprecated to return an error message to update tooling to use validator_wasm.js and it will no longer provide validation. This was released for a short time on Monday, September 13, 2021 before being rollbacked. It will be released again on Thursday, September 23, 2021 pending approval here.

Developers unable to migrate to the WebAssembly validator immediately due to compatibility issues, will be able to utilize a temporary fallback to the native JavaScript validator (validator.20211101.deprecated.js). This will be removed on November 1, 2021.

Motivation

The AMPHTML Validator has two implementations. One in C++ and one in JavaScript. These utilize different HTML parsers which at times can cause differences between validation results of documents. Over time these differences have been reduced. However, there is ongoing complexity and maintenance costs in keeping these two implementations in sync. With a WebAssembly validator version released, it is no longer necessary to continue to endure those costs by deprecating the native JavaScript version in favor of the WebAssembly version.

Impact on Existing Users

Developers utilizing validator.js directly from the Google AMP Cache will need to upgrade to using the WebAssembly version (validator_wasm.js). A temporary fallback version is provided until November 1, 2021.

Alternative Implementation

A temporary fallback version is provided until November 1, 2021 via https://cdn.ampproject.org/v0/validator.20211101.deprecated.js. Ideally they would upgrade to https://cdn.ampproject.org/v0/validator_wasm.js or utilize an existing developer tool such as the Node.js Package.

Additional Context

Example of messaging returned by the deprecated native JavaScript validator via Node.js Package:

$ amphtml-validator --validator.js minimum_valid_amp.html

minimum_valid_amp.html:1:0 The native JavaScript AMPHTML Validator (validator.js) has been turned down. If you are seeing this error, update your tooling to instead load the API compatible WebAssembly AMPHTML Validator (validator_wasm.js) instead.

Example of messaging returned by the deprecated native JavaScript validator via DevTools:

> amp.validator.validateString('test');

validator.js:7 ERROR: The native JavaScript AMPHTML Validator (validator.js) has been turned down. If you are seeing this error, update your tooling to instead load the API compatible WebAssembly AMPHTML Validator (validator_wasm.js) instead.

Notifications

/cc @ampproject/wg-approvers

@cramforce
Copy link
Member

Can you explain why this cannot be done as a backwards compatible change? (For clients that can execute wasm)

@honeybadgerdontcare
Copy link
Contributor Author

It could be done. The WebAssembly validator (validator_wasm.js) has the same interfaces as the native validator (validator.js). Because it is a significant change, a new HTML parser and new validator engine, we thought it prudent to make it evident that this is different and the native version will no longer be supported.

@cramforce
Copy link
Member

I think it is more in the spirit of AMP to be backwards compatible if at all possible, and to only require work by embedders if necessary.

Also, my understanding is that on the npm side only the patch version was incremented (which was probably a mistake as it broke my tests from a timeout due to increased startup time of the WASM code)

@Gregable
Copy link
Member

@cramforce keep in mind that this file is not an API layer, has never been described as an API layer in any documentation that I know of, and should probably not be considered an API layer. All of the APIs have been made to be backwards compatible. Embedders should neither be directly embedding this file nor the wasm one.

Which isn't to say I'm opposed to keeping the filename constant, only that it should not really have been an issue either way.

During the transition, there needed to be two simultaneous versions as not all tooling could be migrated in lockstep. Hence the creation of two files. It's not entirely clear that it's safe to switch clients using an undocumented API to a new implementation - we have no way to test it and it could produce unexpected results (like your timeout).

@Gregable
Copy link
Member

@caoboxiao - there is also one incompatibility reason they need to be separate files.

@antiphoton
Copy link
Member

The WebAssembly validator is not 100% backward compatible with the previous native JavaScript validator, the validator_wasm.js adds an async init function, which must be called before all other functions. The developers need to not only change the URL to validator_wasm.js, but also need to add the statement await amp.validator.init();.

The amp.validator.init is async function because the initialization of WebAssembly is async. However, the public APIs (for example amp.validator.validateString) of the previous native JavaScript validator are declared sync functions, hence it is not possible to make the initialization of WebAssembly to be inside of amp.validator.validateString, and we have to introduce the new amp.validator.init.

@cramforce
Copy link
Member

Thanks for the context. That breaking change does not apply to the node.js module, right?

@honeybadgerdontcare
Copy link
Contributor Author

Thanks for the context. That breaking change does not apply to the node.js module, right?

It does not. The devtools under validator/... were updated and released to use the new amp.validator.init. E.g. Chrome Extension #34502 and Node.js #34213.

@jridgewell
Copy link
Contributor

If I'm reading everything right, the node module API remains the same?

amphtmlValidator.getInstance().then(function (validator) {
  validator.validateString()
});

Because getInstance() was always async, it doesn't matter that we now async instantiate a WASM module. The only change is to people who directly fetch the validator via URL. If so, then I approve this change.


I'm interested in what is generating the validator_wasm.js code. Where's the source file?

@antiphoton
Copy link
Member

Yes, amp.validator.init is included in getInstance().

@antiphoton
Copy link
Member

validator_wasm.js is generated in Google3, and not open source yet.

@kristoferbaxter
Copy link
Contributor

validator_wasm.js is generated in Google3, and not open source yet.

We should discuss this item. Why would the default change before the source is open?

@Gregable
Copy link
Member

The source code is available in GitHub and open-source, we just need to get the build script working in GitHub.

@kristoferbaxter
Copy link
Contributor

Oh, that makes sense, thanks for the clarification.

@rsimha Not sure if this is on your radar, but wanted to make sure it was known.

@kristoferbaxter
Copy link
Contributor

Seems like you're looking for approvers.

I approve, once the build system is open-source and others can generate the wasm representation for future JS/WASM hosting.

@antiphoton
Copy link
Member

antiphoton commented Jan 12, 2022

After merging #37367, the building of the WebAssembly version AMP validator is now open-source.

@Gregable
Copy link
Member

@kbax @rsimha for approval now that the requested change is available.

@kristoferbaxter
Copy link
Contributor

Approved.

@rsimha
Copy link
Contributor

rsimha commented Jan 21, 2022

Based on #36110 (comment) and #36110 (comment), I approve this change.

@Gregable
Copy link
Member

Also approved. This is clear to proceed.

@antiphoton
Copy link
Member

https://cdn.ampproject.org/v0/validator.js is now deprecated, and it returns an error The native JavaScript AMPHTML Validator (validator.js) has been turned down for any HTML input.

For developers seeing this error message

  • If you are using validator.js directly, please change the URL to be https://cdn.ampproject.org/v0/validator_wasm.js.
  • If you are using the NPM amphtml-validator, please upgrade to the latest version 1.0.35. (The version 1.0.35 internally uses validator_wasm.js.)

@mszylkowski
Copy link
Contributor

Hi, there seems to be an issue with the Wasm loading / initializing that was reported in #38401 (and partially in #36404 as well), which breaks the NPM package as well as requiring a workaround inside the repo (amp.validation.init) and importing the script that doesn't work in the NPM package.

The API described in the documentation (getInstance().then(instance => instance.validateString())) doesn't work out of the box (check demo in the first link) when using NPM package.

However, when importing directly <script src="https://cdn.ampproject.org/v0/validator_wasm.js"></script>, it's possible to call await window.amp.validator.init(); and get the validation working. I'm specifically referring to the NPM package not working (which doesn't expose window.amp.xxx).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
INTENT TO DEPRECATE Proposes deprecating an existing AMP feature. P2: Soon WG: caching
Projects
None yet
Development

No branches or pull requests

8 participants