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

SWC-7067 #5611

Merged
merged 5 commits into from
Jan 8, 2025
Merged

SWC-7067 #5611

merged 5 commits into from
Jan 8, 2025

Conversation

jay-hodgson
Copy link
Member

No description provided.

@jay-hodgson jay-hodgson requested a review from nickgros January 8, 2025 01:09
public class CardConfiguration {

String type;
double secondaryLabelLimit;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
double secondaryLabelLimit;
long secondaryLabelLimit;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did something change, I thought long values were disallowed? Or perhaps I'm misunderstanding.
From https://www.gwtproject.org/doc/latest/DevGuideCodingBasicsJSNI.html

The Java long type cannot be represented in JavaScript as a numeric type, so GWT emulates it using an opaque data structure. This means that JSNI methods cannot process a long as a numeric type. The compiler therefore disallows, by default, directly accessing a long from JSNI: JSNI methods cannot have long as a parameter type or a return type, and they cannot access a long using a JSNI reference.

Copy link
Contributor

@nickgros nickgros Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is JsInterop, not JSNI. However, it seems like double is the least problematic primitive type for JsInterop. In JsInterop, int and long do work; we're using those for properties in other JsInterop types. I cannot find an authoritative document that describes exactly what Java primitives and classes JsInterop supports.

Keeping double is probably fine!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! I'm still thinking in JSNI. I guess I'll leave it, for the small potential performance gain

Copy link
Contributor

@nickgros nickgros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small primitive type change but overall looks good

@jay-hodgson jay-hodgson merged commit 3617cc9 into Sage-Bionetworks:develop Jan 8, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants