Open Kilian opened 5 years ago
My first thought is that it's something related to local resource. IIRC we are filtering out resources that do not start with http(s).
@antross what do you think?
@Kilian Thanks for the suggestion! This seems like a great scenario for us to support.
If you want to dig deeper you can create your own custom build of the extension by following the steps at the top of the CONTRIBUTING file for the browser extension. The initial build of the repo takes some time (10-20min), but after that you can rebuild directly from the packages/extension-browser
folder by running yarn build
(which should take less than a minute).
You'll probably want to start by looking at the expected call to _onComplete
and work your way back to figure out why it's not getting called.
My first thought is that it's something related to local resource. IIRC we are filtering out resources that do not start with http(s).
@molant That could cause us to omit some data, but the scan should still complete shortly after the onLoad
event fires for the document. I checked the code and we should finish even if we never got the raw source for the top-level document.
My first thought is that it's something related to local resource. IIRC we are filtering out resources that do not start with http(s).
@molant That's why I also tested with the <webview>
, which has a regular url as origin.
@antross I'll see what I can do there, thanks!
I've been trying to get this to run (on Ubuntu 18.04) folloing the contributing guidelines but I'm running into issues.
My initial yarn build
is really fast:
build all packages
Verifying if repo does not have any changes
Downloading revision "cdaa672fb1ad081ee224aa339d2a48d468259061"
Unzipping
Deleting cdaa672fb1ad081ee224aa339d2a48d468259061.zip
Done!
build time: 0m:2s
Done in 13.14s.
but a yarn build
in the extension-browser folder spits out this:
An additional yarn
in that directory will say everything is ok though:
➜ extension-browser git:(master) yarn
yarn install v1.19.1
[1/5] Validating package.json...
[2/5] Resolving packages...
success Already up-to-date.
Done in 1.11s.
So I think i'm missing a step here. I'm using master
. Any suggestions?
I'm looking into this now see if I can repro.
In the meantime, the order of commands is:
yarn
--> This should download all packages and create any necessary symbolic links.yarn build
--> Will download the artifacts if already available or otherwise fallback to build, so it can be really fast (otherwise it can be 10+ minutes).I can repro this locally. Looks like some dist
folders don't get zipped.
@Kilian working on a solution. CI is currently running to validate the changes and I hope to merge the change within the next couple hours 🤞
https://github.com/webhintio/hint/pull/3119 is on the pipeline now and should fix this issue.
Commit just got merged. You can run the command now (it will default to build everything from source as CI hasn't finished yet) or wait a bit until it's done.
It runs now, but it does end with this warning:
WARNING in ../parser-html/dist/src/parser.js
Module not found: Error: Can't resolve 'module' in '/home/kilian/Workspace/OS/hint/packages/parser-html/dist/src'
@ ../parser-html/dist/src/parser.js
@ ./dist/src/content-script/webhint.js
Done in 21.63s.
Is that expected?
@Kilian Yes, that warning is currently expected when we bundle parser-html
using webpack as part of the extension-browser
build.
Electron currently doesn't support two of the Chrome extension API's that Webhint uses in the background script: webNavigation.onCommitted
and browser.tabs.reload
. Electron is currently moving to a new implementation of the Chrome extension API, so I'm checking if it makes sense to add support for these specific calls to the current implementation.
FYI we want to add an option to scan a site without reloading. For this scenario reload and onComitted shouldn't be used so in theory things should then work in electron.
Is that something you'd be interested in looking into?
From: Kilian Valkhof notifications@github.com Sent: Monday, October 21, 2019 03:10 To: webhintio/hint Cc: Antón Molleda; Mention Subject: Re: [webhintio/hint] [Feature] Electron support for browser extension (#3059)
Electron currently doesn't support two of the Chrome extension API's that Webhint uses in the background script: webNavigation.onCommitted and browser.tabs.reload. Electron is currently moving to a new implementation of the Chrome extension API, so I'm checking if it makes sense to add support for these specific calls to the current implementation.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/webhintio/hint/issues/3059?email_source=notifications&email_token=AAEUDAR5CWV33T6OBOAOCLDQPV5ZLA5CNFSM4I4WLPTKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBZZSPI#issuecomment-544446781, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAEUDAVJ3B7BEK6ZCQYZ2GDQPV5ZLANCNFSM4I4WLPTA.
@molant that would mean neither call is required so that should work. It took me a very long time to track this down though, so I'd be up for it if someone could pair with me to talk me through where to make the changes for that.
@Kilian I'd be willing to help here.
I'm wondering if it might first be worth making a modified build that just calls injectContentScript(tabId)
directly in the enable
helper of the background script to see if that's the last missing piece.
If so and we can detect that either onCommited
or browser.tabs.reload
is not available (or detect that we're running in Electron) we could do this automatically. I suspect we may need also to remember that we've already injected the content-script in this case to avoid having multiple listeners on subsequent runs, probably by having the content script itself set some global flag on setup and exit early if it's already set.
And once those two pieces are working we can look into extending the direct call to injectContentScript
be triggered via the UI to support scans without reloading generally.
How does that sound?
Sorry for the late reply. This isn't off my radar. Electron has been working on a new chrome extensions api that changes the way this integration would work, but currently has a fair amount of bugs so I'm waiting for that to stabilize.
🚀 Feature request
Description
Electron support for the browser extension.
When running the Chrome browser extension in Electron, which supports devtools extensions, the scan times out. I don't get any specific error message, also not when inspecting the devtools with a remote session, so I don't know why.
One hint (sorry) is that in the bug report the "url for which webhint failed" seems to be empty, so maybe it's not resolving the right URL. I tested this both with devtools on an Electron window directly (where the url would resolve to a file) and on a
webview
, whose url would resolve to a proper https url, and neither bug report includes the url.What scenarios will this solve?
Running in Electron lets people run webhint on their application code, and would let me run it inside the browser I'm developing, Polypane. If pointed in the right direction, I'd be willing to dig deeper into where the timeout occurs and see if I can fix it.
The below is the generated bug report when using the extension in Electron
🐛 Bug report
Description
Environment
webhint
configurationwebhint’s configuration
Categories: * [ ] Accessibility * [ ] Compatibility * [x] Performance * [ ] Pitfalls * [ ] PWA * [ ] Security Your target browsers: * [x] Recommended settings * [ ] Custom: Ignored resources: * * [x] None * * [ ] Different origin * * [ ] Custom:Debug output
Please include the content of the
Error details
section if an error message was displayed.webhint’s Error details
```text Scan timed out. ```