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

Improve navigation #10

Closed
aslushnikov opened this issue Jun 14, 2017 · 5 comments
Closed

Improve navigation #10

aslushnikov opened this issue Jun 14, 2017 · 5 comments
Assignees

Comments

@aslushnikov
Copy link
Contributor

aslushnikov commented Jun 14, 2017

Navigation is hard.
Make sure the following navigation scenarios work properly:

  • page.navigate('not-a-url') should return false
  • page.navigate('https://expired.badssl.com/') should return false
  • page.navigate('http://example.com/non-existing-page') should return false
  • page.navigate('http://example.com') with no internet should return false
  • page.navigate('data:text/html,hello') should return true
  • Page's navigation via inner javascript's window.location.href = 'http://example.com' should be reported to puppeteer, probably via the Navigated event.

All of this should be also applicable to frame navigation in #4

@aslushnikov
Copy link
Contributor Author

Today, 'page.navigate' waits for the load event to fire. We might need an option to wait for DOMContentLoaded instead, though I don't know a scenario where this is useful.

@JoelEinbinder
Copy link
Collaborator

example.com returns status 200 for all paths, so that non-existing-page case won't work. Is the idea to return false for non-200 status codes? I feel like the true/false should be whether or not the current page was rendered with data from that url. So 403/404/200 would be true, but timeout/brokenssl would be false.

@aslushnikov aslushnikov added this to the v0.1 milestone Jun 27, 2017
@paulirish
Copy link
Collaborator

Rather than false, to me it seems better to reject with an Error with a more descriptive message.

@aslushnikov
Copy link
Contributor Author

So the issue with returning true/false is that it's hard to decide what is a success and what is a failure. For example, navigating to a 404 page might be a valid scenario, since a lot of websites have their own custom-made 404's.

How about:

  • we consider navigation failed if there's a navigation timeout or an interstitial
  • anything else returns a status code for the main resource

This should address both @JoelEinbinder and @paulirish concerns. What do you guys think?

@paulirish
Copy link
Collaborator

it could return the fetch response object for the main resource

aslushnikov added a commit that referenced this issue Jun 28, 2017
This patch adds a test to verify that navigation properly waits for the
network to become idle.

References #10
@aslushnikov aslushnikov self-assigned this Jun 28, 2017
aslushnikov pushed a commit that referenced this issue Jun 28, 2017
This patch:
- Changes network idle promise to wait for 2 or fewer network requests for at least idleTime (defaults to 5s) before resolving.
- Adds timer cleanup to failure navigation case.
- Adds handling of webSocketClosed.
- Ignores unrecognized requestIds to avoid negative inflight requests.

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

No branches or pull requests

3 participants