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

Fix/timeouts #1331

Closed
wants to merge 11 commits into from
Closed

Fix/timeouts #1331

wants to merge 11 commits into from

Conversation

molant
Copy link
Member

@molant molant commented Sep 21, 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)

I'm trying to figure out why Travis timesout after merging ea56a95.

For that I'll split all changes in different commits and see what is the culprit.

@sarvaje
Copy link
Contributor

sarvaje commented Sep 21, 2018

@molant, I think you need to rebase this.

@molant
Copy link
Member Author

molant commented Sep 21, 2018

I think you need to rebase this.

Nope, I've removed the commit and adding them one by one. Travis is building them in my fork.

@sarvaje
Copy link
Contributor

sarvaje commented Sep 21, 2018

but you have conflicts XD

alrra and others added 8 commits September 21, 2018 13:16
@alrra @molant
🚀 parser-html - v1.0.2 [skip ci]
790135a
@alrra @molant
Chore: Update parser-html to v1.0.2 [skip ci]
3d84b7a
@antross @molant
Fix: Disable hints that are currently noisy when run locally
4d7706a 
Disable `hint-apple-touch-icons` because it is not aware if the
project is a PWA or not (needs to check for a manifest).

Disable `webpack-config/module-esnext-typescript` because it
requires loading both the webpack and typescript configs at the
same time to work (which doesn't currently happen in VS Code).

These can be re-enabled once the affected hints are updated.

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Close #1329
@molant
Fix: Convert to text base64 text based responses
870e599 
The body of some responses returns as base64 by the Debugging Protocol
even though they are text based. This happens a lot with the JavaScript
of ads and causes the JavaScript parser to fail.
@molant
Fix: Location issues with jsdom
b80fbfb 
Depending on the package manager used for the installation, different
versions of `parse5` with different APIs could be installed.
Until #1274 is merged, this solves for that case.
@molant
Chore: Add types to debugging protocol
32f6449
@molant
Chore: Change utf8 to utf-8
f9e32de 
Node supports both. Updated for consistency.
@molant
Chore: Prefix all private properties with _
fda932e
@molant
Copy link
Member Author

molant commented Sep 21, 2018

This should be ready. I've changed the history removing the previous CDP changes and @alrra's commit changing the parser tests. I've also split the commits in multiple to make it easier to see what is going on.

Ideally this should be forced pushed into master. I don't think anything has been added since I branched.

@molant
Copy link
Member Author

molant commented Sep 21, 2018

For the curious, this is the travis run in my branch: https://travis-ci.org/molant/hint/builds/431682392

@molant
Copy link
Member Author

molant commented Sep 22, 2018

Latest travis run https://travis-ci.org/molant/hint/builds/431748090

@molant
Copy link
Member Author

molant commented Sep 23, 2018

@alrra, I'm almost sure the timeouts we are having right now are because travis is using the latest npm version that still has the base64 error. Locally they are passing.

alrra added a commit that referenced this pull request Sep 23, 2018
@alrra
Fix: Revert "Fix: Several issues with the Debugging Protocol"
4b9a18e 
This reverts:

 * ea56a95.
 * b6b4321.

as they make things timeout.

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Ref #1331
alrra added a commit that referenced this pull request Sep 23, 2018
@alrra
Fix: Revert "Fix: Several issues with the Debugging Protocol"
85496fe 
This reverts:

 * ea56a95
 * b6b4321

as they make things timeout.

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Ref #1331
@alrra
Copy link
Contributor

alrra commented Sep 23, 2018

@molant I cannot make this work locally. :(

Until we figure this out, I reverted the changes.

@molant
Copy link
Member Author

molant commented Sep 24, 2018 via email

@alrra
Copy link
Contributor

alrra commented Sep 24, 2018

Are you getting timeouts or something different locally?

timeouts

@molant
Fix: Truncated body in some circumstances
d5811ef 
To make sure the body is available, `getResponseBody` has to be called
after the `loadingFinished` event is received for the `requestId`.

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Fix #1325
@molant
Copy link
Member Author

molant commented Sep 24, 2018

Closing in favor of #1337

@molant molant closed this Sep 24, 2018
@molant molant deleted the fix/timeouts branch September 26, 2018 16:37
@dependabot-preview dependabot-preview bot mentioned this pull request Apr 19, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alrra alrra Awaiting requested review from alrra

@sarvaje sarvaje Awaiting requested review from sarvaje

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@molant @sarvaje @alrra @antross