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

Issues calling javascript object methods directly vs callMethod #49399

Open
jbrownsw opened this issue Jul 5, 2022 · 3 comments
Open

Issues calling javascript object methods directly vs callMethod #49399

jbrownsw opened this issue Jul 5, 2022 · 3 comments
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-js-interop Issues that impact all js interop

Comments

@jbrownsw
Copy link

jbrownsw commented Jul 5, 2022

  • Dart SDK Version: 2.17.5
  • I was using Chrome on Windows, but I don't think this is applicable because it's really related to the javascript being generated

I have an open PR for grpc-dart to implement a fetch transport rather than XHR. I just recently realized the code wasn't working in release builds. I had to change the code from calling methods directly on a dynamic object that represents a javascript ReadableStream to using callMethod. The original code worked fine while debugging, but it wasn't until building in release and deploying it that I realized it didn't work.

Here's a link to the commit that works around the issue:
grpc/grpc-dart@5a8818b

The dart code in question from the PR looks something like this (from fetch_transport.dart):

  dynamic _response;
  dynamic get body =>
      _response != null ? js_util.getProperty(_response, 'body') : null;
...
    if (status < 200 || status >= 300) {
      onErrorController.add(status);
    }

    final reader = body?.getReader();
    if (reader == null) {
      onErrorController.add(0);
      return;
    }

This generates the following javascript:

if(p.gbv(p)<200||p.gbv(p)>=300)p.at.u(0,p.gbv(p))
o=p.ch
o=o!=null?o.body:null
if(o!=null) o.aNq()

The result is a type error o.aNq is not a function. That's not all that readable so I generated it with --dart-define=Dart2jsOptimization=O0 which results in the following snippet:

if ($async$self.get$status($async$self) < 200 || $async$self.get$status($async$self) >= 300)
  $async$self.onErrorController.add$1(0, $async$self.get$status($async$self));
t1 = $async$self._response;
t1 = t1 != null ? t1.body : null;
if (t1 != null)
  t1.getReader$0();
$async$self.onErrorController.add$1(0, 0);
// goto return
$async$goto = 1;

This is quite a bit more readable, but it still fails on the getReader$0() call because this should just be getReader(). If I change the code to use callMethod this resolves the issue, but results in more code and is less readable. As you can see from the commit I also had to stop calling read directly on the reader and instead use callMethod there too. My biggest complaint is really just the inconsistency between debugging and release though because having it's much easier to debug/troubleshoot it during dev than after it's been deployed.

The final javascript with my changes (Note that it actually just calls stream.getReader() as expected):

if ($async$self.get$status($async$self) < 200 || $async$self.get$status($async$self) >= 300)
  $async$self.onErrorController.add$1(0, $async$self.get$status($async$self));
t2 = $async$self._response;
stream = t2 != null ? t2.body : null;
reader = stream != null ? stream.getReader() : null;
if (reader == null) {
  $async$self.onErrorController.add$1(0, 0);
  // goto return
  $async$goto = 1;
  break;
}
@lrhn lrhn added the area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. label Jul 6, 2022
@mraleph
Copy link
Member

mraleph commented Jul 6, 2022

I think this is working as intended (though it's an unfortunate discrepancy between dart2js and DDC).

Calling methods on dynamic is supposed to only work for Dart methods on Dart objects. If you want to call a JS method on a JS object you need to go through interop. This is because Dart methods and JS functions have different calling conventions.

@jbrownsw
Copy link
Author

jbrownsw commented Jul 6, 2022

That makes sense. Then to me the issue is that it works in debug at all. I'm fairly new to dart/flutter and VERY new to flutter web so this definitely caught me off guard and was unexpected.

@joshualitt joshualitt added the web-js-interop Issues that impact all js interop label Jul 8, 2022
@srujzs
Copy link
Contributor

srujzs commented Jul 8, 2022

@mraleph is right here - dynamic invocations are meant for Dart objects. You have to be explicit about the typing for JS interop objects.

As for the discrepancy, I do agree it's not great. While we lose some flexibility by tying ourselves down to a consistent calling syntax in JS, there have been several bugs around interop objects (dart:html types especially) due to the calling syntax being the same between Dart and non-Dart objects in DDC. I'd need to talk with @nshahan about this, but I wonder if we can work towards changing the call sites to look like how they do with dart2js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-js-interop Issues that impact all js interop
Projects
None yet
Development

No branches or pull requests

5 participants