Skip to content

JS interop makes non-breaking JS changes break Dart code #48186

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

Open
nex3 opened this issue Jan 20, 2022 · 3 comments
Open

JS interop makes non-breaking JS changes break Dart code #48186

nex3 opened this issue Jan 20, 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

@nex3
Copy link
Member

nex3 commented Jan 20, 2022

In JS, it's legal to pass more arguments to a function than that function declares. For example, (() => {})(1) is valid code. In Dart this is not legal, and (() {})(1) will produce an error.

The issue comes when passing Dart functions to JS. Despite needing to wrap the JS functions in allowInterop(), Dart doesn't actually allow them to take multiple arguments. This violates JS libraries' assumptions about what changes are non-breaking, and leads to real-world breakages such as sass/dart-sass#1625.

@sigmundch
Copy link
Member

@srujzs - in thinking about the other issue (high-order conversions with ExternalDartObjectReference in function parameters), I realized that a similar idea would be helpful to address this issue. That is: to switch from a single generic allow-interop implementation, to mutliple specialized implementations depending on the static type of the argument to allow interop.

Those specialized implementations would no longer need to use Function.apply in the body, making them a bit leaner and faster. We'd also be able to accept and ignore additional arguments at that point.

@srujzs
Copy link
Contributor

srujzs commented Apr 1, 2024

Oh I briefly mentioned this in the other issue (#55342) before I saw this, but I agree. :)

copybara-service bot pushed a commit that referenced this issue Jun 26, 2024
…n.toDart

Lowers these extension methods to some helper functions instead of
allowInterop to improve performance and get consistent semantics.

Specifically:
- Function.toJS no longer gives you the same JSFunction when calling
toJS on the same function multiple times in the JS compilers.
- Adds fast calling syntax for functions with 0-5 args in the JS
compilers.
- Allows additional args to be passed to converted functions that
are omitted in the JS compilers.
- The static type now determines the number of args that can be
passed to the JS function in the JS compilers.
- Fixes an issue in dart2wasm where if too few arguments are
passed, the call may succeed due to conversion of undefined to
null.
- Adds throws when a user tries to wrap a JS function that
returned from Function.toJS in the JS compilers.

Closes #55515
Addresses #48186

CoreLibraryReviewExempt: Changes to docs only in API surface.
Change-Id: I41d864dc5e02b597d9f1c16c88e3c04872f28225
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/368065
Reviewed-by: Nicholas Shahan <nshahan@google.com>
Reviewed-by: Stephen Adams <sra@google.com>
Commit-Queue: Srujan Gaddam <srujzs@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>
@srujzs
Copy link
Contributor

srujzs commented Jun 28, 2024

FYI @nex3 - this has landed with Function.toJS. I'll leave this open since we haven't addressed the same issue with allowInterop yet. That would be a bit more complicated since we can't rely on static type information only.

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
Status: No status
Development

No branches or pull requests

4 participants