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

Remove unnecessary use of dynamics #652

Merged
merged 18 commits into from Oct 12, 2018

Conversation

Projects
None yet
3 participants
@tommymonk
Contributor

tommymonk commented Sep 28, 2018

The use of dynamic throughout the code made it difficult to safely make a change relating to #650
Dynamics are terrible for hiding refactor mistakes until run-time.

I've plumbed through JObject for response and JToken for method interface, this clarity allowed me to do a lot of streamlining of the response processing and in some places identify where we are serialize/de-serialize the same data many times.

I didn't want to add to the string debt so I pulled out a large amount of the key strings into a Consts class to help with quality and refactor and reduce risk of future mistake.

As discussed in #650 it seemed like a sensible idea to remove the 'dynamic' or 'object overloads of methods like EvaluateFunctionAsync which are a cause of confusion, however if someone wants to use the type 'object' they will now get the correct types serialized for numerics.

All of the existing tests are passing for me, I wanted to add extra tests to cover #650 but wasn't sure which test class to use, maybe you could guide me ?

@Meir017

basically when the response isn't used you don't need to specify the type (<object>) just use the JObject overload

Show resolved Hide resolved lib/PuppeteerSharp/Constants.cs Outdated
Show resolved Hide resolved lib/PuppeteerSharp/CDPSession.cs
"document.cookie = 'doSomethingOnlyOnce=true; expires=Fri, 31 Dec 9999 23:59:59 GMT'");
}
using (var browser2 = await Puppeteer.LaunchAsync(options, TestConstants.LoggerFactory))
{
var page2 = await browser2.NewPageAsync();
await page2.GoToAsync(TestConstants.EmptyPage);
Assert.Equal("doSomethingOnlyOnce=true", await page2.EvaluateExpressionAsync("document.cookie"));
Assert.Equal("doSomethingOnlyOnce=true", await page2.EvaluateExpressionAsync<object>("document.cookie"));

This comment has been minimized.

@Meir017

Meir017 Sep 28, 2018

Collaborator

this should probably be EvaluateExpressionAsync<string>

@@ -156,15 +156,15 @@ public async Task UserDataDirOptionShouldRestoreCookies()
{
var page = await browser.NewPageAsync();
await page.GoToAsync(TestConstants.EmptyPage);
await page.EvaluateExpressionAsync(
await page.EvaluateExpressionAsync<object>(

This comment has been minimized.

@Meir017

Meir017 Sep 28, 2018

Collaborator

can this use the JObject overload without specifying <object>?

This comment has been minimized.

@tommymonk

tommymonk Sep 28, 2018

Contributor

As I mentioned in the PR description, I thought by removing the default object and dynamic methods we would make users think about what they want, I can add back methods defaulting to object though ?

}
using (var browser2 = await Puppeteer.LaunchAsync(options, TestConstants.LoggerFactory))
{
var page2 = await browser2.NewPageAsync();
await page2.GoToAsync(TestConstants.EmptyPage);
Assert.Equal("hello", await page2.EvaluateExpressionAsync("localStorage.hey"));
Assert.Equal("hello", await page2.EvaluateExpressionAsync<object>("localStorage.hey"));

This comment has been minimized.

@Meir017

Meir017 Sep 28, 2018

Collaborator

this should be <string> not <object>

@@ -402,7 +402,7 @@ public async Task ShouldSelectTheTextWithMouse()
await Page.FocusAsync("textarea");
const string text = "This is the text that we are going to try to select. Let's see how it goes.";
await Page.Keyboard.TypeAsync(text);
await Page.EvaluateExpressionAsync("document.querySelector('textarea').scrollTop = 0");
await Page.EvaluateExpressionAsync<object>("document.querySelector('textarea').scrollTop = 0");

This comment has been minimized.

@Meir017

Meir017 Sep 28, 2018

Collaborator

can this use the JObject overload without specifying <object> ?

@@ -448,7 +448,7 @@ public async Task ShouldFireContextmenuEventOnRightClick()
public async Task ShouldSetModifierKeysOnClick()
{
await Page.GoToAsync(TestConstants.ServerUrl + "/input/scrollable.html");
await Page.EvaluateExpressionAsync("document.querySelector('#button-3').addEventListener('mousedown', e => window.lastEvent = e, true)");
await Page.EvaluateExpressionAsync<object>("document.querySelector('#button-3').addEventListener('mousedown', e => window.lastEvent = e, true)");

This comment has been minimized.

@Meir017

Meir017 Sep 28, 2018

Collaborator

can this use the JObject overload without specifying <object> ?

@@ -476,7 +476,7 @@ public async Task ShouldSpecifyRepeatProperty()
{
await Page.GoToAsync(TestConstants.ServerUrl + "/input/textarea.html");
await Page.FocusAsync("textarea");
await Page.EvaluateExpressionAsync("document.querySelector('textarea').addEventListener('keydown', e => window.lastEvent = e, true)");
await Page.EvaluateExpressionAsync<object>("document.querySelector('textarea').addEventListener('keydown', e => window.lastEvent = e, true)");

This comment has been minimized.

@Meir017

Meir017 Sep 28, 2018

Collaborator

can this use the JObject overload without specifying <object> ?

@@ -505,7 +505,7 @@ public async Task ShouldClickLinksWhichCauseNavigation()
public async Task ShouldTweenMouseMovement()
{
await Page.Mouse.MoveAsync(100, 100);
await Page.EvaluateExpressionAsync(@"{
await Page.EvaluateExpressionAsync<object>(@"{

This comment has been minimized.

@Meir017

Meir017 Sep 28, 2018

Collaborator

can this use the JObject overload without specifying <object> ?

@@ -580,7 +580,7 @@ public async Task ShouldTypeAllKindsOfCharacters()
public async Task ShouldSpecifyLocation()
{
await Page.GoToAsync(TestConstants.ServerUrl + "/input/textarea.html");
await Page.EvaluateExpressionAsync(@"{
await Page.EvaluateExpressionAsync<object>(@"{

This comment has been minimized.

@Meir017

Meir017 Sep 28, 2018

Collaborator

can this use the JObject overload without specifying <object> ?

@kblok

Let's make this change first so we get an smaller PR to review.

@@ -17,7 +17,7 @@ public static async Task Main(string[] args)
using (var browser = await Puppeteer.LaunchAsync(options))
using (var page = await browser.NewPageAsync())
{
await page.EvaluateFunctionAsync("_dumpioTextToLog => console.log(_dumpioTextToLog)", args[0]);
await page.EvaluateFunctionAsync<object>("_dumpioTextToLog => console.log(_dumpioTextToLog)", args[0]);

This comment has been minimized.

@kblok

kblok Sep 28, 2018

Owner

We need to support "void" functions. That function won't return a value. I don't think we have to force the user to pick a Type he won't need.

This comment applies to many changes here. If we don't need a result, we don't need a generic.

This comment has been minimized.

@tommymonk

tommymonk Sep 28, 2018

Contributor

Agreed

@Meir017

this should return Task<JToken>

/// <summary>
/// Executes a script in browser context
/// </summary>
/// <param name="script">Script to be evaluated in browser context</param>
/// <remarks>
/// If the script, returns a Promise, then the method would wait for the promise to resolve and return its value.
/// </remarks>
/// <seealso cref="EvaluateFunctionAsync(string, object[])"/>
/// <seealso cref="EvaluateFunctionAsync{T}(string, object[])"/>
/// <seealso cref="EvaluateExpressionHandleAsync(string)"/>
/// <returns>Task which resolves to script return value</returns>
public Task<object> EvaluateExpressionAsync(string script)

This comment has been minimized.

@Meir017

Meir017 Sep 28, 2018

Collaborator

this should return Task<JToken>

@kblok

This comment has been minimized.

Owner

kblok commented Sep 28, 2018

@tommymonk there are still some tests that need to be reverted. On the other hand, this PR is conflicted. Could you resolve the conflicts? So the CI works on this PR?

BTW, thanks for the calories burned on this PR!

@tommymonk

This comment has been minimized.

Contributor

tommymonk commented Sep 28, 2018

I'm almost finished on the overloads the the Evaluate* methods but I've got test failures because the JToken now returned can 'empty' rather than null as confirmed by it's HasValues property. Would you rather I check for the HasValues property and return null, or task the calling code (tests and users) with checking this property ?

@kblok

This comment has been minimized.

Owner

kblok commented Sep 28, 2018

@tommymonk now the Evaluate* with not generic returns a Task<object>. I would by changing that to Task. So if you use EvaluateFunctionAsync("") we assume that you don't need the value. If you do need it, you should do EvaluateFunctionAsync<Foo>("").

We could also play smart and check if the user wants the raw JSON token EvaluateFunctionAsync<JToken>("") so if T is JToken we return the JSON object.

@tommymonk

This comment has been minimized.

Contributor

tommymonk commented Sep 28, 2018

Unfortuantely I can't have overloads of these methods that return nothing (Task) as well as Task<JToken> because of course C# doesn't allow this with the same method signature.
I have chosen to go with Task<JToken> because then at least the caller can choose to not even acknowledge the return value

@tommymonk tommymonk force-pushed the tommymonk:features/remove-dynamic-overuse branch from 4910dc6 to 06447e4 Sep 28, 2018

@tommymonk

This comment has been minimized.

Contributor

tommymonk commented Sep 28, 2018

@kblok rebased atop your commits today

@kblok

Looking good, Getting rid of dynamics was something we always wanted to do and it's quite a big change but it worth the effort.

@@ -89,7 +89,8 @@ public async Task ShouldClickWrappedLinks()
public async Task ShouldClickOnCheckboxInputAndToggle()
{
await Page.GoToAsync(TestConstants.ServerUrl + "/input/checkbox.html");
Assert.Null(await Page.EvaluateExpressionAsync("result.check"));
var foo = await Page.EvaluateExpressionAsync("result.check");
Assert.Null(foo);

This comment has been minimized.

@kblok

kblok Sep 28, 2018

Owner

Let's undo this change we don't need it.

This comment has been minimized.

@tommymonk

tommymonk Sep 28, 2018

Contributor

Forgot to take it out when debugging

@@ -13,25 +13,25 @@ public class BoundingBox : IEquatable<BoundingBox>
/// The x coordinate of the element in pixels.
/// </summary>
/// <value>The x.</value>
[JsonProperty("x")]
[JsonProperty(Constants.X)]

This comment has been minimized.

@kblok

kblok Sep 28, 2018

Owner

Let's keep JsonProperties as strings it doesn't add much value to use constants here.

This comment has been minimized.

@tommymonk

tommymonk Sep 28, 2018

Contributor

You're suggesting undoing all the work I put into removing strings scattered throughout the code ?

This comment has been minimized.

@kblok

kblok Sep 28, 2018

Owner

Yes, sorry about that. It's not something I think we need to do, and it's out of the scope of removing dynamics.

Show resolved Hide resolved lib/PuppeteerSharp/Browser.cs Outdated
Show resolved Hide resolved lib/PuppeteerSharp/Constants.cs Outdated
@tommymonk

This comment has been minimized.

Contributor

tommymonk commented Sep 28, 2018

Is there a reason that you don't just use BaristaLabs to generate all your poco objects for each of the devtools APIs, seems like this would take care of Response types you're wanting ?

https://github.com/BaristaLabs/chrome-dev-tools-generator

@kblok

This comment has been minimized.

Owner

kblok commented Sep 28, 2018

@tommymonk, basically we were growing little by little not even knowing that this project would get any interest. We started with dynamics, and then we began moving to classes, adding small classes as we needed.
Now I have smart people like you suggesting apps I never heard before 😂
I will take a look at that app.

@kblok

Looking good

@@ -72,7 +73,7 @@ public async Task ShouldBeAbleToDetachSession()
var client = await Page.Target.CreateCDPSessionAsync();
await client.SendAsync("Runtime.enable");
var evalResponse = await client.SendAsync("Runtime.evaluate", new { expression = "1 + 2", returnByValue = true });
Assert.Equal(3, evalResponse.result.value.ToObject<int>());
Assert.Equal(3, evalResponse["result"]["value"].ToObject<int>());

This comment has been minimized.

@kblok

kblok Sep 29, 2018

Owner

Just thinking about an example. Would a user be able to do this?

dynamic evalResponse = await client.SendAsync<ExpandoObject>("Runtime.evaluate", new { expression = "1 + 2", returnByValue = true });
Assert.Equal(3, evalResponse.result.value.ToObject<int>());

This comment has been minimized.

@tommymonk

tommymonk Sep 29, 2018

Contributor

Even easier;

dynamic evalResponse = await client.SendAsync("Runtime.evaluate", new { expression = "1 + 2", returnByValue = true });
Assert.Equal(3, evalResponse.result.value.ToObject<int>());

Or

var evalResponse = await client.SendAsync("Runtime.evaluate", new { expression = "1 + 2", returnByValue = true });
Assert.Equal(3, evalResponse.SelectToken("result.value"));
Show resolved Hide resolved lib/PuppeteerSharp/CDPSession.cs Outdated
Show resolved Hide resolved lib/PuppeteerSharp/Connection.cs Outdated
Show resolved Hide resolved lib/PuppeteerSharp/Constants.cs Outdated
Show resolved Hide resolved lib/PuppeteerSharp/ExecutionContext.cs Outdated
@@ -1,30 +1,31 @@
using System.Collections.Generic;

This comment has been minimized.

@kblok

kblok Sep 29, 2018

Owner

Same here

@@ -0,0 +1,21 @@
using Newtonsoft.Json.Linq;

This comment has been minimized.

@kblok

kblok Sep 29, 2018

Owner

Let's move this to the Helper folder

@@ -1,39 +1,39 @@
using System.Collections.Generic;

This comment has been minimized.

@kblok

kblok Sep 29, 2018

Owner

Let's reduce the changelog here

@@ -1,16 +1,16 @@
using System;

This comment has been minimized.

@kblok

kblok Sep 29, 2018

Owner

Let's reduce the changelog

Show resolved Hide resolved lib/PuppeteerSharp/Response.cs Outdated

@tommymonk tommymonk force-pushed the tommymonk:features/remove-dynamic-overuse branch from 1adfd23 to b3a7dd8 Sep 29, 2018

Tommy Monk added some commits Oct 8, 2018

Tommy Monk
Tommy Monk
@tommymonk

This comment has been minimized.

Contributor

tommymonk commented Oct 8, 2018

Removed line-ending changes to the files mentioned, merged in your master changes.

@kblok

I love this progress. Great job!

Show resolved Hide resolved lib/PuppeteerSharp.Tests/PageTests/SetRequestInterceptionTests.cs Outdated
Show resolved Hide resolved lib/PuppeteerSharp/CDPSession.cs Outdated
Show resolved Hide resolved lib/PuppeteerSharp/Dialog.cs Outdated
Show resolved Hide resolved lib/PuppeteerSharp/ElementHandle.cs
Show resolved Hide resolved lib/PuppeteerSharp/ExecutionContext.cs Outdated
Show resolved Hide resolved lib/PuppeteerSharp/ExecutionContext.cs Outdated
Show resolved Hide resolved lib/PuppeteerSharp/Frame.cs Outdated
Show resolved Hide resolved lib/PuppeteerSharp/Frame.cs Outdated
Show resolved Hide resolved lib/PuppeteerSharp/Input/Mouse.cs Outdated
Show resolved Hide resolved lib/PuppeteerSharp/Page.cs Outdated
@tommymonk

This comment has been minimized.

Contributor

tommymonk commented Oct 10, 2018

Just wanted to confirm you aren't waiting on me for any further rework ?

@kblok

This comment has been minimized.

Owner

kblok commented Oct 10, 2018

@tommymonk cool I didn't know it was ready again.

Show resolved Hide resolved lib/PuppeteerSharp/Dialog.cs Outdated
Show resolved Hide resolved lib/PuppeteerSharp/Frame.cs Outdated
@kblok

This comment has been minimized.

Owner

kblok commented Oct 10, 2018

Besides my Evaluate* concerns, I think this is almost ready to go.
@Meir017 would you mind taking a look at it?

@tommymonk would you mind writing a Breaking Change report so we tell the users what it's changing for them and what they would need to change to get their app working?

@kblok

This comment has been minimized.

Owner

kblok commented Oct 10, 2018

@Meir017 let's try to merge this before any other PR, so we don't conflict our friend @tommymonk

@kblok

kblok approved these changes Oct 10, 2018

@tommymonk

This comment has been minimized.

Contributor

tommymonk commented Oct 10, 2018

I'm bound to have forgotten something, but will update this comment with your feedback.

Should we mention the change of Evaluate* return type changing from dynamic to JToken because the current type being boxed as dynamic is JToken, though ?
Same goes for Response.JsonAsync and CDPSession.SendAsync I guess ?

Use of evaluative methods that previously resolved to a JavaScript number will now return a more appropriate .NET type as discussed in #650

@kblok

This comment has been minimized.

Owner

kblok commented Oct 11, 2018

 @tommymonk think about the code that a user might need to change, so we add that to our release notes.

@kblok kblok merged commit d4c5d05 into kblok:master Oct 12, 2018

2 checks passed

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

This comment has been minimized.

Owner

kblok commented Oct 12, 2018

@tommymonk 💥

@tommymonk

This comment has been minimized.

Contributor

tommymonk commented Oct 12, 2018

Thanks

@tommymonk tommymonk deleted the tommymonk:features/remove-dynamic-overuse branch Oct 12, 2018

@tommymonk

This comment has been minimized.

Contributor

tommymonk commented Oct 12, 2018

I believe that I've found a bug introduced by my PR that isn't covered by test.

await new BrowserFetcher().DownloadAsync(BrowserFetcher.DefaultRevision);

var browser = await new Launcher().LaunchAsync(new LaunchOptions
{
    Headless = true,
});

var page = await browser.NewPageAsync();

var result = await page.EvaluateExpressionAsync("var str = 'asdf'; str;"); // result is null

browser.Dispose();

The return type of EvaluateExpressionAsync isn't specified, the object is string and it return null

Its cause because of this line of code in ExecutionContext;
return result is JToken token && !token.HasValues ? default : result;

Instead of just checking HasValues it needs to use a smarter test like the top answer here

public static bool IsNullOrEmpty(this JToken token)
{
    return (token == null) ||
           (token.Type == JTokenType.Array && !token.HasValues) ||
           (token.Type == JTokenType.Object && !token.HasValues) ||
           (token.Type == JTokenType.String && token.ToString() == String.Empty) ||
           (token.Type == JTokenType.Null);
}
@kblok

This comment has been minimized.

Owner

kblok commented Oct 12, 2018

@tommymonk go for it an add a test :)

@tommymonk

This comment has been minimized.

Contributor

tommymonk commented Oct 12, 2018

My branch is gone now so I'll create a new one and new PR ?

@kblok

This comment has been minimized.

Owner

kblok commented Oct 12, 2018

@tommymonk

This comment has been minimized.

Contributor

tommymonk commented Oct 12, 2018

I'll get on it, can you suggest where is appropriate fore new tests to go ?

@kblok

This comment has been minimized.

Owner

kblok commented Oct 12, 2018

@tommymonk I would go with PageTests.EvaluateTests.ShouldWorkWithoutGenerics()

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