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

RDART-866: Minimal web support #1699

Merged
merged 17 commits into from
Jun 6, 2024
Merged

RDART-866: Minimal web support #1699

merged 17 commits into from
Jun 6, 2024

Conversation

nielsenko
Copy link
Contributor

@nielsenko nielsenko commented May 29, 2024

The aim of this PR is to allow package realm to be included in flutter web projects without breaking compilation. It is not intended to actually make realm work runtime with flutter web.

It is safe to use unmanaged realm object in web projects, except if they include Decimal128 fields.

Fixes: #1374

@cla-bot cla-bot bot added the cla: yes label May 29, 2024
@nielsenko nielsenko force-pushed the kn/minimal-web-support branch 4 times, most recently from 39418d8 to ffd860d Compare May 31, 2024 08:23
Copy link

coveralls-official bot commented May 31, 2024

Pull Request Test Coverage Report for Build 9405734587

Details

  • 330 of 376 (87.77%) changed or added relevant lines in 36 files are covered.
  • 6 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.2%) to 86.797%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/realm_dart/lib/src/configuration.dart 5 6 83.33%
packages/realm_dart/lib/src/handles/native/results_handle.dart 9 10 90.0%
packages/realm_dart/lib/src/handles/native/session_handle.dart 14 15 93.33%
packages/realm_dart/lib/src/handles/native/set_handle.dart 11 12 91.67%
packages/realm_dart/lib/src/handles/native/app_handle.dart 22 24 91.67%
packages/realm_dart/lib/src/handles/native/http_transport_handle.dart 17 19 89.47%
packages/realm_dart/lib/src/handles/native/map_handle.dart 14 16 87.5%
packages/realm_dart/lib/src/handles/native/mutable_subscription_set_handle.dart 6 8 75.0%
packages/realm_dart/lib/src/handles/object_handle.dart 0 2 0.0%
packages/realm_dart/lib/src/realm_object.dart 1 3 33.33%
Files with Coverage Reduction New Missed Lines %
packages/realm_dart/lib/src/app.dart 2 91.3%
packages/realm_dart/lib/src/configuration.dart 4 74.58%
Totals Coverage Status
Change from base Build 9404164866: -0.2%
Covered Lines: 5943
Relevant Lines: 6847

💛 - Coveralls

@nielsenko nielsenko force-pushed the kn/minimal-web-support branch 4 times, most recently from 461fecd to 7cc3038 Compare May 31, 2024 16:27
@nielsenko nielsenko requested a review from nirinchev June 1, 2024 10:22
@nielsenko nielsenko changed the title Minimal web support RDART-866: Minimal web support Jun 1, 2024
@nielsenko nielsenko force-pushed the kn/minimal-web-support branch 8 times, most recently from b88120f to 4b9fd3d Compare June 3, 2024 13:42
@nielsenko nielsenko marked this pull request as ready for review June 3, 2024 13:45
Copy link
Member

@nirinchev nirinchev left a comment

Choose a reason for hiding this comment

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

Looks good - I skimmed over large portions of repetitive changes, but those seem to be covered in tests, so they seem to work. One thing to consider is extracting the web exception somewhere so we don't have to repeat it 58329529 times.

CHANGELOG.md Outdated Show resolved Hide resolved
void createCollection(Realm realm, RealmValue value, Pointer<realm_list> Function() createList, Pointer<realm_dictionary> Function() createMap) {
CollectionHandleBase? collectionHandle;
try {
switch (value.collectionType) {
case RealmCollectionType.list:
final listHandle = ListHandle(createList(), realm.handle);
final listHandle = ListHandle(createList(), realm.handle as RealmHandle);
Copy link
Member

Choose a reason for hiding this comment

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

Rather than cast here, should we move the cast to the ListHandle/MapHandle ctor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not fan of that.. I prefer all methods on the platform specific handles (including ctors) to traffic in platform specific handles. The problem is more in the handle extension method on Realm.

packages/realm_dart/lib/src/handles/native/init.dart Outdated Show resolved Hide resolved
packages/realm_dart/lib/src/realm_class.dart Outdated Show resolved Hide resolved
packages/realm_dart/lib/src/realm_web.dart Outdated Show resolved Hide resolved
packages/realm_dart/test/utils/native/platform_util.dart Outdated Show resolved Hide resolved
@nielsenko
Copy link
Contributor Author

nielsenko commented Jun 6, 2024

Looks good - I skimmed over large portions of repetitive changes, but those seem to be covered in tests, so they seem to work. One thing to consider is extracting the web exception somewhere so we don't have to repeat it 58329529 times.

I have add a base-class on all web handles, and have it implement noSuchMethod. Also the actual throw has been refactored into the support function webNotSupported.

@nielsenko nielsenko force-pushed the kn/minimal-web-support branch 4 times, most recently from 0826f58 to e84d2bd Compare June 6, 2024 10:53
@nirinchev
Copy link
Member

No, Decimal128 won't be supported because it relies on a native implementation. Unfortunately, this type is not implemented in pure dart and it's not obvious that such an implementation would be easy/possible with the primitives we have there.

@dotjon0
Copy link

dotjon0 commented Jun 6, 2024

No, Decimal128 won't be supported because it relies on a native implementation. Unfortunately, this type is not implemented in pure dart and it's not obvious that such an implementation would be easy/possible with the primitives we have there.

could we fallback to a String in the case the RealmModel has a Decimal128 type then (for web only)? We can then deal with it there onwards. Otherwise we are going to have lots of Realm Models we cant use in 'web'.

Are there any other 'Realm' types that will not be supported in 'web'? Or any other limitations (granted Device Sync does not work in web).

@nielsenko
Copy link
Contributor Author

could we fallback to a String in the case the RealmModel has a Decimal128 type then (for web only)? We can then deal with it there onwards. Otherwise we are going to have lots of Realm Models we cant use in 'web'.

No. The generator run before we know if the model will be used on web or not.

@dotjon0
Copy link

dotjon0 commented Jun 6, 2024

could we fallback to a String in the case the RealmModel has a Decimal128 type then (for web only)? We can then deal with it there onwards. Otherwise we are going to have lots of Realm Models we cant use in 'web'.

No. The generator run before we know if the model will be used on web or not.

this package (which I think you use?) https://pub.dev/packages/bson supports Decimal128 and web?

how about property annotations that takes into account the Flutter target web VS anything else (native) to conditionally build the models, for example:

@RealmModel
class _SalesOrder {

  @PrimaryKey()
  late ObjectId id;

  @IgnoredWeb() /// this property is not included for 'web'
  late ObjectId salesOrderTotalPrice;

  @IgoredNative() /// this property is not included for 'native' (macOs/Windows/iOS/Android/etc)
  late String salesOrderTotalPrice;
}

Other than this, if we Realm cant conditionally build based on flutter target we are going to get lots of models with type issues...so web will not work...

@nielsenko
Copy link
Contributor Author

Other than this, if we Realm cant conditionally build based on flutter target we are going to get lots of models with type issues...so web will not work...

We will add web support for Decimal128 based on https://pub.dev/packages/decimal

@dotjon0
Copy link

dotjon0 commented Jun 6, 2024

Other than this, if we Realm cant conditionally build based on flutter target we are going to get lots of models with type issues...so web will not work...

We will add web support for Decimal128 based on https://pub.dev/packages/decimal

Thank you sooooo much @nielsenko - are they any other limitations in 'web' for Realm that we should be aware of? Obviously DeviceSync will not work in 'web'.

@nielsenko nielsenko force-pushed the kn/minimal-web-support branch from c1145f5 to 4d9f2ce Compare June 6, 2024 16:23
@nielsenko nielsenko force-pushed the kn/minimal-web-support branch from 4d9f2ce to 1e8e180 Compare June 6, 2024 18:07
@nielsenko nielsenko merged commit abb8ed1 into main Jun 6, 2024
57 checks passed
@nielsenko nielsenko deleted the kn/minimal-web-support branch June 6, 2024 20:58
@nielsenko
Copy link
Contributor Author

This is now released with realm: ^3.0.0

nirinchev added a commit that referenced this pull request Jun 12, 2024
* main:
  Add vNext Changelog header (#1717)
  [Release 3.0.0] (#1716)
  libraryVersion moved to realm_libary.dart (take 2)
  libraryVersion moved to realm_libary.dart
  Update CHANGELOG (#1715)
  Github composite action for setting up flutter on runner (#1710)
  RDART-866: kn/decimal128 web support (#1713)
  Reduce expected gain of memEquals for test stability
  Refresh after awaiting download to stabilize tests
  RDART-866: Minimal web support (#1699)
  RDART-1052: Update realm-core to v14.9.0 (#1704)
  RDART-1020: Fix writeAsync behaviour (#1666)
  RDART-999: Fix flutter test dlopen (#1623)
  RDART-1045: Expose setrlimit ios (#1700)
  RDART-962: Use xcode 15.4 (#1548)
  RDART-1039: Drop catalyst support. Flutter doesn't support it (#1696)

# Conflicts:
#	packages/realm_dart/src/realm-core
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ignore Realm from Web.
3 participants