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.

Sign up for GitHub

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

Jump to bottom

Audit doctype #5213

Closed
jakearchibald opened this issue May 15, 2018 · 14 comments
Closed

Audit doctype #5213

jakearchibald opened this issue May 15, 2018 · 14 comments
Labels
feature good first issue needs-priority new_audit

Comments

@jakearchibald
Copy link

Recommend <!DOCTYPE html> to avoid quirks mode.

https://developer.mozilla.org/en-US/docs/Glossary/Doctype
@mathiasbynens
Copy link
Member

+9001 to this. It’s surprisingly common to still see web pages without one.

@schalkneethling
Copy link

Hey there @jakearchibald, are their some docs I can use to learn how to contribute a new audit to lighthouse? Would love to work on this one. Thanks!

@mathiasbynens
Copy link
Member

README.md and CONTRIBUTING.md have some info, and it might help to look at an existing audit that just checks for stuff in the DOM. Here’s an example: https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/audits/seo/hreflang.js

For this particular audit, you could just check if document.doctype is not null.

@brendankenny
Copy link
Member

brendankenny commented May 15, 2018 •
edited
Loading

if there's a document.doctype, looks like you probably also want to check that document.doctype.name is 'html'?

For this you're going to need a gatherer and an audit. Generally the gatherer will collect the information on the doctype from the browser and the audit will do the actual correctness testing. hreflang is a pretty complicated audit compared to this :) so a better example might be the meta-description audit.

The PR for that audit (#3227) lays out pretty well how to get a gatherer and audit to work together, how to add to them to a config, unit tests needed, etc (though most of the SEO category stuff won't be necessary...it would just go into Best Practices in the default config).

Let us know how things go!

@schalkneethling
Copy link

Thanks for all of the input everyone. I am going to dig into this. Will let you know should I run into any problems.

@schalkneethling
Copy link

@mathiasbynens @brendankenny Would this audit go inside dobetterweb? Thanks!

@schalkneethling
Copy link

I do not see the following file in the current codebase so, I am guessing a file such as this is no longer required?

https://github.com/GoogleChrome/lighthouse/pull/3227/files#diff-4ba93e471ddd053783bd7729348f18cd

Because this line already exists https://github.com/GoogleChrome/lighthouse/pull/3227/files#diff-37b42887961ae2bfa58b7e227cf7a672R229 I probably do not need to add an entry in this file either? Thanks in advance.

@midzer
Copy link
Contributor

midzer commented May 18, 2018 •
edited
Loading

Yes, fs.readdirSync(path.join(__dirname, './audits/dobetterweb')).map(f => 'dobetterweb/${f}') should read all files in /dobetterweb automatically.

@schalkneethling
Copy link

For requiredArtifacts inside the meta function, passing Doctype is clearly not correct. I poked around and some of the other audits but, they use a variety of values here. What would be the correct one to use for this audit? Thanks!

@schalkneethling
Copy link

Would it help if I opened a WIP PR so folks can see whether I am going in the right direction? @midzer

@midzer
Copy link
Contributor

midzer commented May 18, 2018

@schalkneethling I have not implemented an audit for lighthouse yet, just a minor contribution. Would have to dig deep into this one as well, you've been first to answer this open issue.

I think thats more a question to regular contributors/maintainers if they assist you in a WIP PR.

@schalkneethling
Copy link

@brendankenny Thoughts?

@patrickhulce
Copy link
Collaborator

@schalkneethling yeah go ahead and open up a PR and we can discuss impl details there :)

@schalkneethling schalkneethling mentioned this issue May 19, 2018
Merged
@schalkneethling
Copy link

Opened the PR: #5274

When running the test I get the following which to me suggests that what I am passing to requiredArtifacts is not what I should be passing. There might very well be other issues as well. Your assistance is appreciated. Excited to get to know how this all fits together:

$ tsc -p .
lighthouse-core/audits/dobetterweb/doctype.js(15,5): error TS2322: Type '{ name: string; description: string; failureDescription: string; helpText: string; requiredArtifa...' is not assignable to type 'Meta'.
  Types of property 'requiredArtifacts' are incompatible.
    Type '"Doctype"[]' is not assignable to type '("Manifest" | "URL" | "settings" | "LighthouseRunWarnings" | "fetchTime" | "UserAgent" | "traces"...'.
      Type '"Doctype"' is not assignable to type '"Manifest" | "URL" | "settings" | "LighthouseRunWarnings" | "fetchTime" | "UserAgent" | "traces" ...'.
lighthouse-core/audits/dobetterweb/doctype.js(31,19): error TS2339: Property 'Doctype' does not exist on type 'Artifacts'.
lighthouse-core/audits/dobetterweb/doctype.js(37,19): error TS2339: Property 'Doctype' does not exist on type 'Artifacts'.
lighthouse-core/gather/gatherers/dobetterweb/doctype.js(13,36): error TS2339: Property 'Doctype' does not exist on type 'Artifacts'.
lighthouse-core/gather/gatherers/dobetterweb/doctype.js(18,19): error TS2339: Property 'getDocument' does not exist on type 'Driver'.
lighthouse-core/gather/gatherers/dobetterweb/doctype.js(18,38): error TS7006: Parameter 'node' implicitly has an 'any' type.
lighthouse-core/scripts/update-report-fixtures.js(18,91): error TS2339: Property 'port' does not exist on type 'string | AddressInfo'.
  Property 'port' does not exist on type 'string'.
Files:           343
Lines:        106462
Nodes:        454373
Identifiers:  154460
Symbols:      165501
Types:         61685
Memory used: 233999K
I/O read:      0.08s
I/O write:     0.00s
Parse time:    1.08s
Bind time:     0.46s
Check time:    2.88s
Emit time:     0.00s
Total time:    4.42s
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
npm ERR! Test failed.  See above for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature good first issue needs-priority new_audit
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@mathiasbynens @jakearchibald @brendankenny @patrickhulce @midzer @schalkneethling