Skip to content

[breaking change] Change return type of isTruthy and not #55267

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

Closed
srujzs opened this issue Mar 21, 2024 · 7 comments
Closed

[breaking change] Change return type of isTruthy and not #55267

srujzs opened this issue Mar 21, 2024 · 7 comments
Assignees
Labels
area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes web-js-interop Issues that impact all js interop

Comments

@srujzs
Copy link
Contributor

srujzs commented Mar 21, 2024

Some more context here: #55238. This is the "reverse" of that change - move all operators to use JS types instead of returning Dart types.

Intended Change

dart:js_interop's isTruthy and not should now return a JSBoolean instead of a bool.

dart:js_interop exposes isTruthy and not operators to be consistent with dart:js_util API. They currently return a bool, as the result is always a boolean JS value.

Justification

JS operator utility members should use JS types instead of their Dart equivalents for performance. They are likely to be used when the conversion is deemed costly (otherwise, users could just convert to the Dart value and use Dart operators) and they are likely to be composed on one other e.g. b1.and(b2.or(b3)). In the case of composition, this adds several additional unneeded conversions.

Impact

Minimal. There are no usages in google3, in Flutter, in the SDK, or on GitHub.

Mitigation

Convert the resulting JSBoolean to a boolean with toDart:

bool b1 = true.toJS.not;
bool b2 = true.toJS.isTruthy;
bool b1 = true.toJS.not.toDart;
bool b2 = true.toJS.isTruthy.toDart;

cc @sigmundch

@srujzs srujzs added web-js-interop Issues that impact all js interop breaking-change-request This tracks requests for feedback on breaking changes area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop labels Mar 21, 2024
copybara-service bot pushed a commit that referenced this issue Mar 25, 2024
Related issues:
#55024
#55267

These operators were initially broken in 3.3 and were exposed
as returning JSBoolean but implemented as returning bool. They
were fixed to return bool in the public API, but we should
prefer to have them return JS types as they're likely to be used
in cases where implicit conversions are not useful.

CoreLibraryReviewExempt: Fixing type mismatch in backend-specific library.
Change-Id: I3b0e60550dcac78918f8399d11238dcfa34982cd
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/359180
Commit-Queue: Srujan Gaddam <[email protected]>
Reviewed-by: Sigmund Cherem <[email protected]>
@sigmundch
Copy link
Member

LGTM 👍, this would bring all operators to have a consistent API: all other operators today return a JS type, this would make the dart:js_interop library homogeneous.

cc @lrhn @kevmoo

cc @itsjustkevin - for the breaking change process


In terms of timing, I'm open to either having this in 3.4 or 3.5, given the cutoff date, I'm fine waiting until 3.5. Usage is low enough that we don't expect much impact later either, but we may want to suggest a backward compatible way to write code if we don't release it in 3.4. For example:

bool isReallyTruthy(JSAny a) {
  Object o = a.isThruthy;
  return o is bool ? o : (o as JSBoolean).toDart;
}

@itsjustkevin
Copy link
Contributor

@vsmenon @Hixie @grouma for review.

@itsjustkevin itsjustkevin self-assigned this Mar 26, 2024
@grouma
Copy link
Member

grouma commented Mar 26, 2024

SGTM.

@itsjustkevin
Copy link
Contributor

Pinging @vsmenon and @Hixie

@Hixie
Copy link
Contributor

Hixie commented May 21, 2024

no objection; seems reasonable but i don't have enough context to fully evaluate.

@vsmenon
Copy link
Member

vsmenon commented Jun 6, 2024

lgtm

@itsjustkevin
Copy link
Contributor

Breaking change approved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web-js Issues related to JavaScript support for Dart Web, including DDC, dart2js, and JS interop breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes web-js-interop Issues that impact all js interop
Projects
Status: Complete
Development

No branches or pull requests

6 participants