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

New: Add a standalone `parser-html` #1277

Closed
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@antross
Member

antross commented Aug 28, 2018

Pull request checklist

Make sure you:

For non-trivial changes, please make sure you also:

  • Added/Updated related documentation.
  • Added/Updated related tests.

Short description of the change(s)

For use by connector-local in support of #1254 so we can expand the
set of hints which run in the VSCode extension.

Uses jsdom (with scripts and resource loading disabled) to maximize
support for existing hints (e.g. hint-axe).

@antross antross requested review from alrra, molant and sarvaje as code owners Aug 28, 2018

@antross antross referenced this pull request Aug 28, 2018

Closed

New: Add extension for VSCode #1254

14 of 15 tasks complete
@@ -242,6 +245,17 @@ export default class LocalConnector implements IConnector {
});
}
private watchForWindow() {

This comment has been minimized.

@sarvaje

sarvaje Aug 28, 2018

Member

This will work if there is more than one html file?

This comment has been minimized.

@antross

antross Aug 28, 2018

Member

This will only populate the connector for the target HTML file as the connector APIs are tied to that singleton as far as I could tell.

This comment has been minimized.

@antross

antross Aug 28, 2018

Member

To further clarify, if connector-local is used with a glob pattern matching multiple HTML files, only the first one parsed will be used to populate DOM-related connector APIs.

Note in the context of the VS Code extension, connector-local will be called separately for each file so this is not an issue there.

This comment has been minimized.

@sarvaje

sarvaje Aug 28, 2018

Member

Yep, the problem will be with a glob pattern.

I think you need to touch a little bit how the connector works to support multiple HTMLs.

Right now, the fetch is in parallel, but maybe you can refactor the code to emit the fetch::end::html serially so you can set the properties in the connector for each html, this way, the connector will support the parse-html for all the HTML files. Do you know what I mean?

The idea is when the connector store all the HTMLs in an array, without emits the fetch::end::html. Then, iterate that array serially doing:

  1. emit the fetch::end::html
  2. in the event parse::html set this._window
  3. wait until the analysis of the html finish.
  4. clean this._window
  5. go to the next html.

This comment has been minimized.

@antross

antross Aug 29, 2018

Member

Makes sense and doesn't look too difficult from glancing at the code. I'll see if I can incorporate this tomorrow.

@@ -1,4 +1,5 @@
import { IAsyncHTMLDocument, IAsyncHTMLElement } from './async-html'; //eslint-disable-line
import { IAsyncHTMLDocument, IAsyncHTMLElement, IAsyncWindow } from './async-html'; //eslint-disable-line

This comment has been minimized.

@sarvaje

sarvaje Aug 28, 2018

Member

What does it happen if you remove //eslint-disable-line?

This comment has been minimized.

@antross

antross Aug 28, 2018

Member

Nothing it seems. Want me to remove it? I was originally only making necessary changes (as this comment was already in the file), but it's an easy change so I can include the removal in this PR.

This comment has been minimized.

@sarvaje

sarvaje Aug 28, 2018

Member

Yes please. I saw it wasn't your change, but it would be nice to remove it :)

"watch:test": "ava --watch",
"watch:ts": "npm run build:ts -- --watch"
},
"version": "0.0.1"

This comment has been minimized.

@alrra

alrra Aug 28, 2018

Member

1.0.0

@@ -7,6 +7,7 @@
"timeout": "1m"
},
"dependencies": {
"@hint/parser-html": "^0.0.1",

This comment has been minimized.

@alrra

alrra Aug 28, 2018

Member

Note to self: Move this into a different commit.

This comment has been minimized.

@antross

antross Aug 28, 2018

Member

Honestly that should be a dev dependency as it's only used for types (specifically the HTMLParse event). The only functional tie connector-local has to the parser-html is via the parse::html::end event. I'll fix it (though that probably doesn't change your need to add even the dev dependency in a separate commit).

This comment has been minimized.

@alrra

alrra Aug 28, 2018

Member

I was referring a different commit, not anything about code changes.

That is for the release process.

this._window = null;
const setWindow = (event: HTMLParse) => {
this._window = event.window;

This comment has been minimized.

@antross

antross Aug 28, 2018

Member

This also needs to emit a can-evaluate::script event here or hint-axe and friends won't work. I'll update the PR for this in a bit (need to grab a bite to eat).

@alrra alrra force-pushed the webhintio:master branch from f5ab526 to 8e00cfd Aug 28, 2018

@antross

This comment was marked as resolved.

Member

antross commented Aug 29, 2018

Now when I'm trying to run the connector-local tests on my machine, they're all failing with the message:

'Attempted to wrap default which is already wrapped'

in

\hint\node_modules\sinon\lib\sinon\util\core\wrap-method.js:38.

Any idea what might be up?

@molant

This comment was marked as resolved.

Member

molant commented Aug 29, 2018

That happens if you are trying to create a spy (or similar) twice on the same method without restoring before.

Ive seen that happening before when a test fails and the after portion where the restore happens doesn't get executed (or it gets executed but fails for some reason before doing it). The next test will fail with that message.

@antross antross force-pushed the antross:parser-html branch from 7a0e6e1 to 02f1bb1 Aug 29, 2018

@antross

This comment has been minimized.

Member

antross commented Aug 29, 2018

FYI, I merged and tested this with the VSCode extension PR and it worked (although error positions will need a bit of work as expected). Even got hint-axe running after changing its HintScope to any.

@molant

This comment has been minimized.

Member

molant commented Aug 29, 2018

Even got hint-axe running after changing its HintScope to any.

🎉🎉🎉

@molant

This comment has been minimized.

Member

molant commented Aug 30, 2018

Everything is failing because nyc can't be found 🤔

Error: Cannot find module 'C:\projects\hint\packages\parser-html\node_modules\nyc\bin\nyc.js'
    at Function.Module._resolveFilename (module.js:547:15)
    at Function.Module._load (module.js:474:25)
    at Function.Module.runMain (module.js:693:10)
@molant

This looks great. I just have a couple comments but otherwise great job. @sarvaje, @alrra can you take a look when you have time please?

Note: This parser is not needed if using a [connector][connectors]
which connects to a browser as the browser's DOM will be used to
generate events instead.

This comment has been minimized.

@molant

molant Aug 30, 2018

Member

The jsdom connector doesn't need it neither. Maybe reformulate to say it's only required by local?

const resource = this._url = fetchEnd.response.url;
const html = fetchEnd.response.body.content;
const dom = new JSDOM(html, { runScripts: 'outside-only' });

This comment has been minimized.

@molant

molant Aug 30, 2018

Member

Maybe add a comment here explaining why we want outside-only for future us?

await this.engine.emitAsync(`traverse::up`, traverseEvent);
return Promise.resolve();

This comment has been minimized.

@molant

molant Aug 30, 2018

Member

You are already using await through this method so you shouldn't need to return a Promise. Is TS complaining?

@sarvaje

LGTM

@molant

molant approved these changes Aug 31, 2018

@@ -0,0 +1,60 @@
# HTML parser (`@hint/parser-html`)
The `HTML` parser is built on top of `[jsdom][jsdom]` so hints can

This comment has been minimized.

@alrra

alrra Sep 3, 2018

Member
[`jsdom`][jsdom]
},
"description": "webhint parser needed to analyze HTML files",
"devDependencies": {
"@types/cheerio": "^0.22.9",

This comment has been minimized.

@alrra

alrra Sep 3, 2018

Member

This is no longer needed?

"@types/cheerio": "^0.22.9",
"@types/debug": "0.0.30",
"@types/jsdom": "^11.0.6",
"@types/node": "8.0.14",

This comment has been minimized.

@alrra

alrra Sep 3, 2018

Member

Remove types that are not necessary.

(we try to keep them to a minimum, because they often break our builds 😔)

This comment has been minimized.

@alrra

alrra Sep 3, 2018

Member

Also, after you do these changes, can you run yarn in the root so that yarn.lock file gets updated. Thanks!

});
});
test.serial('If fetch::end::html is received, then we should parse the code and emit a parse::html::end event', async (t) => {

This comment has been minimized.

@alrra

alrra Sep 3, 2018

Member
If 'fetch::end::html' is received, then the code should be parsed and the `parse::html::end` event emitted
@antross

This comment has been minimized.

Member

antross commented Sep 4, 2018

@alrra do you want me to rebase and resolve the yarn.lock conflicts?

@alrra

This comment has been minimized.

Member

alrra commented Sep 4, 2018

do you want me to rebase and resolve the yarn.lock conflicts?

Yes, thanks!


Just remove the yarn.lock and run yarn, don't do it manually.

antross added some commits Aug 27, 2018

New: Add a standalone `parser-html`
For use by `connector-local`. Currently uses `cheerio`, but may need
to be cut-over to `jsdom` (with scripts and resource loading disabled)
to maximize support for existing hints (e.g. `hint-axe`).
Chore: Use parser-html data from connector-local
Currently requires parser-html to be explicitly add to the config for
the data to be populated.
Chore: Switch `html-parser` to `jsdom`
Includes changes to `connector-local` to implement `evaluate` and
`querySelectorAll` based upon the new backing.
Chore: Address PR feedback and fix tests
I missed adding a property on the new `IAsyncWindow` interface
which was causing the tests to fail. I thought I had re-run the tests
after that change, but apparently not.
Chore: Fix test failure due to missing API from mock
I started using `engine.on` to listen for `parse::html::end`. Since this
wasn't in the mock for `t.context.engine` one of the early tests
failed (and the rest generated the error `'Attempted to wrap default
which is already wrapped'` since the test didn't clean up.
Chore: Support multiple HTML files in `connector-local`
Accomplished by keeping the fetch in parallel, but emitting the
`fetch:🔚:${type}` events serially so we can have the correct
internal `_window` reference during `hint` execution for each file
(per David's suggestion).

Also emit the `can-evaluate::script` event so `hint-axe` and others
which need to inject script will do so.

antross added some commits Aug 30, 2018

Chore: Address PR feedback
Also fix JSDOM options to include source locations for elements.
Chore: Fix test failures in PR
Turns out `test-only` wasn't defined. I'll be fixing this in the
template for `create-parser` in a separate commit.
Chore: Include `test-only` in `create-parser` template
Conditional on `#if official` (per Anton's recommendation) since
it's only needed in the CI for `hint` itself.
Chore: Remove obsolete dependencies
Includes updating `yarn.lock`.

@antross antross force-pushed the antross:parser-html branch from 484b1a2 to 6519ebf Sep 4, 2018

@alrra alrra closed this in a7ca732 Sep 5, 2018

@alrra

This comment has been minimized.

Member

alrra commented Sep 5, 2018

@antross antross deleted the antross:parser-html branch Oct 14, 2018

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