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

amphtml-validator returns this.sandbox.amp.validator.categorizeError is not a function #25188

Closed
anuragphadke opened this issue Oct 22, 2019 · 35 comments · Fixed by #25224
Closed

Comments

@anuragphadke
Copy link

anuragphadke commented Oct 22, 2019

##What's the issue?
Running amphtml-validator via command line returns

this.sandbox.amp.validator.categorizeError is not a function

How do we reproduce the issue?

  1. install the latest amphtml-validator package from
  2. run
    amphtml-validator file.html #this file is attached as file.zip in this ticket.
    file.html.zip
    returns:
    this.sandbox.amp.validator.categorizeError is not a function

What browsers are affected?

amphtml-validator via command line is affected

@Sevenaka
Copy link

same problem, is there a solution?

@joselcc
Copy link

joselcc commented Oct 22, 2019

it seems we need to upgrade to the latest version 7d7b8d6#diff-5185ebf96af6c84dd9d0f25168752664L279

@anuragphadke
Copy link
Author

Tried on 1.0.26 which is the latest and was released 3 days ago, same error.

@joselcc
Copy link

joselcc commented Oct 22, 2019

yeah, I still see the old code in v1.0.26, I think they meant to publish v1.0.25

@jackbillstrom
Copy link

Man this is really putting me in a bad position, how the f*ck did they manage to do this....

@Jeiwan
Copy link
Contributor

Jeiwan commented Oct 22, 2019

I'm using version 1.0.21 and it's also broken, but it worked a few hours ago.

@kevva
Copy link

kevva commented Oct 22, 2019

It's broken because amphtml-validator tries to run https://cdn.ampproject.org/v0/validator.js which doesn't contain the said function anymore.

@joselcc
Copy link

joselcc commented Oct 22, 2019

all amphtml-validator versions use what is published in https://cdn.ampproject.org/v0/validator.js, categorizeError method was removed here from the validator engine

nicl added a commit to guardian/dotcom-rendering that referenced this issue Oct 22, 2019
@jackbillstrom
Copy link

jackbillstrom commented Oct 22, 2019

Oh I am going mental over this... [censored..]

my apologies.

@kevva
Copy link

kevva commented Oct 22, 2019

@jackbillstrom, that's not productive at all. You can still remove the line locally for amphtml-validator in your node_modules folder. That's what I did until they fix it.

Bugs will happen from time to time. You don't have to be a script kiddie on an internship to cause them, even if you work at Google.

@liorbashan
Copy link

@kevva , Can you give more details as for what changes can be done locally in node_modules until (or if) they fix this?

@jackbillstrom
Copy link

@kevva Productive or not, I couldn't understand on to get passed this bug. Of course I then come with some hard-on questions.

Thus, as you say, bugs always occur but this was just so annoying.

Thanks. 😈

@kevva
Copy link

kevva commented Oct 22, 2019

@liorbashan, in node_modules/amphtml-validator/index.js on line 279 or something like that.

@liorbashan
Copy link

@kevva , Thank you :)
This is obviously a temporary solution.
Still wondering why would they remove the error documentations from this service, since it was really useful. (more useful than amp.dev documentations :))

@sakinee
Copy link

sakinee commented Oct 22, 2019

What can be done towards the emergence of the amp cache wedding videos for some videos does not work properly at the scales amp.
For example ; https://pornoyakala.com/amp/ why this raya gidrdinmi size is not bigger 16/9?

@cpapazian
Copy link
Contributor

all amphtml-validator versions use what is published in https://cdn.ampproject.org/v0/validator.js, categorizeError method was removed here from the validator engine

that is unfortunate. there really should be a way to run validation while only depending on local code. filed #25200 to track this.

@mrjoro
Copy link
Member

mrjoro commented Oct 22, 2019

/cc @Gregable @ampproject/wg-caching

@amaltas amaltas assigned amaltas and unassigned Gregable Oct 22, 2019
@nainar
Copy link
Contributor

nainar commented Oct 22, 2019

@jackbillstrom your comments above are violating AMP's Code of Conduct which aim to promote civil discourse on all AMP channels.

Please ensure that all future comments follow the Code of Conduct. We understand that the current situation is upsetting, however, personally attacking any of our other members is not pushing discussion forward.

@amaltas
Copy link
Contributor

amaltas commented Oct 22, 2019

Validator error categories and all the references to amp.validator.categorizeError were removed #25134 validator rollup.

Publishing a updated amphtml-validator npm package now.

@amaltas
Copy link
Contributor

amaltas commented Oct 22, 2019

amphtml-validator version 1.0.27 published. Please update the amphtml-validator dependencies to use latest version.

In future, we will make sure any changes in validator.js do not break nodejs package and/or get caught during the release.

Please reopen the issue if you still see the error, or I missed something. PS: This is my first npm publish.

@amaltas amaltas closed this as completed Oct 22, 2019
@cpapazian
Copy link
Contributor

1.0.27 requires node 12, so i am still blocked. what's the context on the minimum node version requirement?

@twifkak
Copy link
Member

twifkak commented Oct 22, 2019

Fix for that is pending in #25208.

@twifkak
Copy link
Member

twifkak commented Oct 22, 2019

Fix published as https://www.npmjs.com/package/amphtml-validator version 1.0.28.

@dmouse
Copy link

dmouse commented Oct 23, 2019

any plans to support node 8.x? Next.js use it vercel/next.js#9172

@rodolphonetto
Copy link

rodolphonetto commented Oct 23, 2019

Maybe I'm not so much smart.. but even after updating the package to 1.0.28 I'm still facing the same error... I use it as dependency of next.js amp pages. I went to line 279 as said before and found that code, is it the last version?

Validator.prototype.validateString = function(inputString, htmlFormat) {
  const internalResult =
      this.sandbox.amp.validator.validateString(inputString, htmlFormat);
  const result = new ValidationResult();
  result.status = internalResult.status;
  for (let ii = 0; ii < internalResult.errors.length; ii++) {
    const internalError = internalResult.errors[ii];
    const error = new ValidationError();
    error.severity = internalError.severity;
    error.line = internalError.line;
    error.col = internalError.col;
    error.message =
        this.sandbox.amp.validator.renderErrorMessage(internalError);
    error.specUrl = internalError.specUrl;
    error.code = internalError.code;
    error.params = internalError.params;
    result.errors.push(error);
  }
  return result;
};

@Timer
Copy link

Timer commented Oct 23, 2019

@rodolphonetto Don't worry! This is still broken despite this issue being closed. We're trying to work with the AMP team on a resolution that doesn't leave all users broken!

@rodolphonetto
Copy link

Ahh ok! I thought version 1.0.28 solved all problems, Where can I watch to be updated about that problem?

@honeybadgerdontcare
Copy link
Contributor

@rodolphonetto @Timer Sorry you're still experiencing issues around this change. We're working on addressing them. Could you verify that this is happening with 1.0.28?

@cpapazian
Copy link
Contributor

@rodolphonetto @Timer Sorry you're still experiencing issues around this change. We're working on addressing them. Could you verify that this is happening with 1.0.28?

@honeybadgerdontcare 1.0.28 moves the minimum node version to 10, so it is still broken on 8, or any other previous versions.

@rodolphonetto
Copy link

@rodolphonetto @Timer Sorry you're still experiencing issues around this change. We're working on addressing them. Could you verify that this is happening with 1.0.28?

There is no problem, I can wait :)

Yes, I'm still facing the same issue with version 1.0.28, my node version is 12.10.0

@Gregable
Copy link
Member

All interfaces should be returning errors/warnings now. This includes previous versions of NPM (sync back to 1.0.23 if you have node compatibility issues).

Note that all errors are categorized as UNKNOWN. The error category system has retired. For some history, this system categorized all errors into an set of 10 broad enums like "GENERIC" and "AUTHOR_STYLESHEET_PROBLEM". Note that category is distinct from severity, which is of the set ERROR/WARNING. Severity is still present and unchanged. There was little if any remaining usage of these categories. Google Search console was one of the remaining users, but no longer consumes categories. Any older version of the validator that still consumes categories will now categorize all errors as UNKNOWN. We'll follow up with fixes to those interfaces to ensure they remove references to categories as well.

@Gregable
Copy link
Member

For browser interfaces you may need to force reload https://cdn.ampproject.org/v0/validator.js

@Timer
Copy link

Timer commented Oct 23, 2019

Thank you for the detailed explanation, distinction between severity and category, and the fix @Gregable et al.

FWIW Next.js CI re-enabled the AMP tests after this began passing again. x-ref vercel/next.js#9162

@rodolphonetto
Copy link

All working fine right now, thank you so much :)

@Gregable
Copy link
Member

FYI as a follow up, the release process for cdn.ampproject.org/v0/validator.js now includes an automated step that runs light verification tests against the latest amphtml-validator NPM version. This would have caught the specific release failure mentioned in this github issue.

This step would not have caught the separate issue with removing support for older Node versions, but that was an explicit decision rather than an unintended bug, which isn't really what tests are there to catch. The consequences of that decision were not fully understood, but I think we understand them better now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.