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

Extension point enabling custom web sockets #574

Merged
merged 4 commits into from Sep 4, 2018

Conversation

Projects
None yet
2 participants
@ggeurts
Contributor

ggeurts commented Aug 28, 2018

Microsoft's default WebSocket implementation does not work on Windows 7 and Windows 2008 R2 platforms. Alternative web sockets implementation exist that do work on those platforms. Therefore it makes sense to add an extension point, so library users can use Puppeteer on these platforms, without Puppeteer becoming dependent on other WebSocket implementations.

@ggeurts ggeurts changed the title from Extension point for use custom web sockets to Extension point enabling custom web sockets Aug 28, 2018

@ggeurts

This comment has been minimized.

Contributor

ggeurts commented Aug 28, 2018

This pull request addresses issue #512.

dir,
"bin",
build,
"net471");

This comment has been minimized.

@kblok

kblok Sep 1, 2018

Owner

Let's retore the tabs here.

var result = new System.Net.WebSockets.ClientWebSocket();
#pragma warning disable 618
result.Options.KeepAliveInterval = TimeSpan.FromMilliseconds(options.KeepAliveInterval);
#pragma warning restore 618

This comment has been minimized.

@kblok

kblok Sep 1, 2018

Owner

Why are we doing this?

This comment has been minimized.

@ggeurts

ggeurts Sep 4, 2018

Contributor

KeepAliveInterval is marked obsolete

/// </summary>
public static readonly WebSocketFactory DefaultWebSocketFactory = async (uri, options, cancellationToken) =>
{
var result = new System.Net.WebSockets.ClientWebSocket();

This comment has been minimized.

@kblok

kblok Sep 1, 2018

Owner

How about adding a using?

/// <param name="options">Web socket creation options.</param>
/// <param name="cancellationToken">Cancellation token.</param>
/// <returns>A <see cref="WebSocket"/> that is connected to the given <paramref name="uri"/>.</returns>
public delegate Task<WebSocket> WebSocketFactory(Uri uri, IWebSocketOptions options, CancellationToken cancellationToken);

This comment has been minimized.

@kblok

kblok Sep 1, 2018

Owner

I'd just declare that as an Action instead of creating a delegate.

This comment has been minimized.

@ggeurts

ggeurts Sep 4, 2018

Contributor

That would become a Func<Uri, IWebSocketOptions, CancellationToken, Task<WebSocket>> which is significantly less self-documenting than the proposed delegate.

/// <summary>
/// Options for <see cref="WebSocket"/> creation.
/// </summary>
public interface IWebSocketOptions

This comment has been minimized.

@kblok

kblok Sep 1, 2018

Owner

Do we have any class which is IWebSocketOptions but it's not IConnectionOptions?

This comment has been minimized.

@ggeurts

ggeurts Sep 4, 2018

Contributor

No, we don't. IConnectionOptions inherits IWebSocketOptions. IWebSocketOptions defines options for WebSocketFactory delegate, which is a subset of what is required by the Connection class. My preference was to keep WebSocketFactory delegate as independent as possible of the rest of the code.

@ggeurts ggeurts force-pushed the ggeurts:websocket branch from f28bcca to 6ad94cc Sep 4, 2018

@ggeurts ggeurts force-pushed the ggeurts:websocket branch from 6ad94cc to 3d25b09 Sep 4, 2018

@kblok

Great job man.
Let's remove the assignment and this is ready to go!

Show resolved Hide resolved lib/PuppeteerSharp/Connection.cs Outdated
@kblok

This comment has been minimized.

Owner

kblok commented Sep 4, 2018

We have a real fail in AppVeyor

@ggeurts

This comment has been minimized.

Contributor

ggeurts commented Sep 4, 2018

The AppVeyor fail is race between Connection.OnClose and Connection.SendAsync around accessing the Connection._responses collection.

@kblok

This comment has been minimized.

Owner

kblok commented Sep 4, 2018

@ggeurts We can't merge a red build, so we'd need a complete solution.

@ggeurts

This comment has been minimized.

Contributor

ggeurts commented Sep 4, 2018

Fine, I will commit a fix

@kblok

kblok approved these changes Sep 4, 2018

@kblok kblok merged commit bfb5ba9 into kblok:master Sep 4, 2018

2 checks passed

CodeFactor No issues found.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@ggeurts ggeurts deleted the ggeurts:websocket branch Sep 4, 2018

@ggeurts

This comment has been minimized.

Contributor

ggeurts commented Sep 4, 2018

Many thanks for considering, reviewing and using this contribution!

@kblok

This comment has been minimized.

Owner

kblok commented Sep 5, 2018

Thank for contributing @ggeurts !

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