microsoft / playwright-dotnet

.NET version of the Playwright testing and automation library.
https://playwright.dev/dotnet/
MIT License
2.47k stars 235 forks source link

EvaluateAsync Esprima parsing fails on >ECMA 2016 #1039

Closed nh43de closed 3 years ago

nh43de commented 3 years ago

Some Background: Puppeteer-sharp has two functions for evaluating JS, namely "evaluate function" and "evaluate expression", Playwright consolidated these into a single API, "EvaluateAsync", and uses Esprima-dotnet to parse that function and determine whether it's a function or expression before sending to Chromium. Which is cool. But,

The problem is that Esprima-dotnet doesn't seem to support ECMA Script >2016, so when calling .EvaluateAsync() on an ECMA 2020 that say, uses option chaining e.g. var name = person?.firstName, the C# method throws a parsing exception and does not send to the underlying browser.

Some solutions: a) provide a fallback when Esprima parsing fails, e.g. maybe default fallback to function since this is an easy convention to remember b) pass a boolean / options object for the evaluation that would allow the user to disable Esprima parsing and/or specify whether it's an expression or a function c) Update / replace Esprima .NET to support latest ECMA Script. Easier said than done obviously but these are options.

Current workarounds: escape js string and use eval()

Just my thoughts, and happy to submit a PR if someone can point me in the right direction. Thanks for looking.

kblok commented 3 years ago

Hey, @nh43de could you tell me a little bit more about your use case? Are you importing scripts from other places? Because my first thought would be why no writing ECMA 2016.

nh43de commented 3 years ago

I'm using it for web scraping and I want to evaluate a small script that's hardcoded in my application and return the scraped objects. E.g. in my C# code


//our js script to run on browser
var personScrapeScript = @"() => {
   const els = getEls(); //web elements that we're scraping

   var people = els.map(thisPerson => { //scrape elements and map to array of people
      let name = thisPerson?.name; //ecma 2020 feature
      return { name };
   });

   return people; //return people array back to Playwright
}";

//run the script and serialize the returned object
var people = page.EvaluateAsync<MyPerson[]>(personScrapeScript);

Nothing fancy, just using nice new ECMA script features like optional chaining (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining) which saves me from having to write tedious code. Am currently working around it by compiling the code using another compiler (e.g. TypeScript), then using that code instead.

Surprised that more people aren’t having this problem.

To me it just seems silly to have the method rely on the parsing to succeed by some secondary parsing layer. I get the convenience aspect that it brings but it shouldn’t throw an error just because the outdated ECMA 2016 parser fails to recognize new language features. This is why I would propose that if parsing fails that it sends along as a function. This would be an easy convention to remember.

kblok commented 3 years ago

@nh43de I can review a PR with a bullet-proof way of detecting if a string contains a function or an expression and supporting new ECMA Scripts.

nh43de commented 3 years ago

@kblok a preferred approach would be much simpler:

That way, it becomes a matter of convention: just pass wrapped in a function (when parsing fails). That is, until the Esprima engine can catch up beyond ECMA 2016. Unless I am missing something.

nh43de commented 3 years ago

@kblok a preferred approach would be much simpler:

  • Parse using Esprima as before

    • If parsing succeeds, continue as before

    • If parsing fails

    • Instead of throwing an unhandled parsing exception, pass as a function string

That way, it becomes a matter of convention: just pass wrapped in a function (when parsing fails). That is, until the Esprima engine can catch up beyond ECMA 2016. Unless I am missing something.

@kblok or anyone else, thoughts on this approach?

kblok commented 3 years ago

Instead of throwing an unhandled parsing exception, pass as a function string

I think we could do this. If the script is invalid we will end up with an exception from the driver. We will ship this for v0.190.0

kblok commented 3 years ago

@nh43de I hit a wall upstream. The driver is validating the script and the Node version is not ECMA 2020 compatible yet. So, although I think this is a valid approach, you won't be able to implement your code yet. I suggest you create an issue upstream.

lahma commented 3 years ago

Commenting on closed issue, but latest Esprima 2 beta has support for also optional chaining https://github.com/sebastienros/esprima-dotnet/releases/tag/v2.0.0-beta-1339 .

pavelfeldman commented 3 years ago

Esprima dependency and function detection logic has been removed!