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

Migrates to package:web and js_interop #2064

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/min_version_test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ jobs:

- uses: subosito/flutter-action@44ac965b96f18d999802d4b807e3256d5a3f9fa1 # pin@v2.16.0
with:
flutter-version: "3.0.0"
flutter-version: "3.13.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's try keeping this and see how it goes without adding the explicit dependency on web:

Suggested change
flutter-version: "3.13.0"
flutter-version: "3.0.0"


- name: Build Android
run: |
Expand All @@ -53,7 +53,7 @@ jobs:

- uses: subosito/flutter-action@44ac965b96f18d999802d4b807e3256d5a3f9fa1 # pin@v2.16.0
with:
flutter-version: "3.0.0"
flutter-version: "3.13.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
flutter-version: "3.13.0"
flutter-version: "3.0.0"


- name: Build iOS
run: |
Expand All @@ -70,7 +70,7 @@ jobs:

- uses: subosito/flutter-action@44ac965b96f18d999802d4b807e3256d5a3f9fa1 # pin@v2.16.0
with:
flutter-version: "3.0.0"
flutter-version: "3.13.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
flutter-version: "3.13.0"
flutter-version: "3.0.0"


- name: Build web
run: |
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
- Bump Cocoa SDK from v8.25.2 to v8.26.0 ([#2060](https://github.com/getsentry/sentry-dart/pull/2060))
- [changelog](https://github.com/getsentry/sentry-cocoa/blob/main/CHANGELOG.md#8260)
- [diff](https://github.com/getsentry/sentry-cocoa/compare/8.25.2...8.26.0)
- Migrates to package:web and js_interop ([#2064](https://github.com/getsentry/sentry-dart/pull/2064))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this should be listed under features, considering it enables WASM compilation


## 8.2.0

Expand Down
1 change: 1 addition & 0 deletions dart/example_web/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ environment:
dependencies:
sentry:
path: ../../dart/
web: ^0.5.1

dev_dependencies:
build_runner: ^2.4.2
Expand Down
33 changes: 23 additions & 10 deletions dart/example_web/web/main.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import 'dart:async';
import 'dart:html';
import 'package:web/web.dart';

import 'package:sentry/sentry.dart';
import 'package:sentry/src/version.dart';
Expand All @@ -24,7 +24,7 @@ Future<void> main() async {
Future<void> runApp() async {
print('runApp');

querySelector('#output')?.text = 'Your Dart app is running.';
document.querySelector('#output')?.text = 'Your Dart app is running.';

await Sentry.addBreadcrumb(
Breadcrumb(
Expand Down Expand Up @@ -57,12 +57,20 @@ Future<void> runApp() async {
);
});

querySelector('#btEvent')
document
.querySelector('#btEvent')
?.onClick
.listen((event) => captureCompleteExampleEvent());
querySelector('#btMessage')?.onClick.listen((event) => captureMessage());
querySelector('#btException')?.onClick.listen((event) => captureException());
querySelector('#btUnhandledException')
document
.querySelector('#btMessage')
?.onClick
.listen((event) => captureMessage());
document
.querySelector('#btException')
?.onClick
.listen((event) => captureException());
document
.querySelector('#btUnhandledException')
?.onClick
.listen((event) => captureUnhandledException());
}
Expand All @@ -76,7 +84,8 @@ Future<void> captureMessage() async {
);
print('capture message result : $sentryId');
if (sentryId != SentryId.empty()) {
querySelector('#messageResult')?.style.display = 'block';
(document.querySelector('#messageResult') as HTMLElement?)?.style.display =
'block';
}
}

Expand All @@ -93,13 +102,16 @@ Future<void> captureException() async {
print('Capture exception : SentryId: $sentryId');

if (sentryId != SentryId.empty()) {
querySelector('#exceptionResult')?.style.display = 'block';
(document.querySelector('#exceptionResult') as HTMLElement?)
?.style
.display = 'block';
}
}
}

Future<void> captureUnhandledException() async {
querySelector('#unhandledResult')?.style.display = 'block';
(document.querySelector('#unhandledResult') as HTMLElement?)?.style.display =
'block';

await buildCard();
}
Expand All @@ -111,7 +123,8 @@ Future<void> captureCompleteExampleEvent() async {
print('Response SentryId: $sentryId');

if (sentryId != SentryId.empty()) {
querySelector('#eventResult')?.style.display = 'block';
(document.querySelector('#eventResult') as HTMLElement?)?.style.display =
'block';
}
}

Expand Down
2 changes: 1 addition & 1 deletion dart/lib/src/environment/environment_variables.dart
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import '../platform_checker.dart';
import '_io_environment_variables.dart'
if (dart.library.html) '_web_environment_variables.dart' as env;
if (dart.library.js_interop) '_web_environment_variables.dart' as env;

/// Reads environment variables from the system.
/// In an Flutter environment these can be set via
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import '../../event_processor.dart';
import '../../sentry_options.dart';
import 'io_enricher_event_processor.dart'
if (dart.library.html) 'web_enricher_event_processor.dart';
if (dart.library.js_interop) 'web_enricher_event_processor.dart';
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible to keep the original code in place for consumers with an older Flutter version? Something like

Suggested change
if (dart.library.js_interop) 'web_enricher_event_processor.dart';
if (dart.library.js_interop) 'web_enricher_event_processor.dart';
if (dart.library.html) 'web_enricher_event_processor_legacy.dart';

Copy link
Contributor

@buenaflor buenaflor Jun 20, 2024

Choose a reason for hiding this comment

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

that works and is also done inside the poc

in your scenario example web_enricher_event_processor_legacy.dart would use dart:html

and 'web_enricher_event_processor.dart would use package:web, it can't use dart:html since that would fail the compilation so we're back at the point where we have to add the web package dependency.

Just looks to me like we can't avoid that unless I see it wrong xd


abstract class EnricherEventProcessor implements EventProcessor {
factory EnricherEventProcessor(SentryOptions options) =>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import 'dart:html' as html show window, Window;
import 'package:web/web.dart' as web show window, Window, Navigator;

import '../../../sentry.dart';
import 'enricher_event_processor.dart';

EnricherEventProcessor enricherEventProcessor(SentryOptions options) {
return WebEnricherEventProcessor(
html.window,
web.window,
options,
);
}
Expand All @@ -16,7 +16,7 @@ class WebEnricherEventProcessor implements EnricherEventProcessor {
this._options,
);

final html.Window _window;
final web.Window _window;

final SentryOptions _options;

Expand Down Expand Up @@ -59,10 +59,9 @@ class WebEnricherEventProcessor implements EnricherEventProcessor {
online: device?.online ?? _window.navigator.onLine,
memorySize: device?.memorySize ?? _getMemorySize(),
orientation: device?.orientation ?? _getScreenOrientation(),
screenHeightPixels: device?.screenHeightPixels ??
_window.screen?.available.height.toInt(),
screenWidthPixels:
device?.screenWidthPixels ?? _window.screen?.available.width.toInt(),
screenHeightPixels:
device?.screenHeightPixels ?? _window.screen.availHeight,
screenWidthPixels: device?.screenWidthPixels ?? _window.screen.availWidth,
screenDensity:
device?.screenDensity ?? _window.devicePixelRatio.toDouble(),
);
Expand All @@ -77,14 +76,12 @@ class WebEnricherEventProcessor implements EnricherEventProcessor {

SentryOrientation? _getScreenOrientation() {
// https://developer.mozilla.org/en-US/docs/Web/API/ScreenOrientation
final screenOrientation = _window.screen?.orientation;
if (screenOrientation != null) {
if (screenOrientation.type?.startsWith('portrait') ?? false) {
return SentryOrientation.portrait;
}
if (screenOrientation.type?.startsWith('landscape') ?? false) {
return SentryOrientation.landscape;
}
final screenOrientation = _window.screen.orientation;
if (screenOrientation.type.startsWith('portrait')) {
return SentryOrientation.portrait;
}
if (screenOrientation.type.startsWith('landscape')) {
return SentryOrientation.landscape;
}
return null;
}
Expand All @@ -101,3 +98,7 @@ class WebEnricherEventProcessor implements EnricherEventProcessor {
);
}
}

extension on web.Navigator {
external double? get deviceMemory;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this used anywhere? If not, we can remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's currently used in the int? _getMemorySize() method. This API has been added to the web package so in a future update it can be removed.

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import '../../event_processor.dart';
import '../../sentry_options.dart';
import 'io_exception_event_processor.dart'
if (dart.library.html) 'web_exception_event_processor.dart';
if (dart.library.js_interop) 'web_exception_event_processor.dart';

abstract class ExceptionEventProcessor implements EventProcessor {
factory ExceptionEventProcessor(SentryOptions options) =>
Expand Down
2 changes: 1 addition & 1 deletion dart/lib/src/origin.dart
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import 'dart:html';
import 'package:web/web.dart';

/// request origin, used for browser stacktrace
String get eventOrigin => '${window.location.origin}/';
12 changes: 6 additions & 6 deletions dart/lib/src/platform/_web_platform.dart
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import 'dart:html' as html;
import 'package:web/web.dart' as web;

import 'platform.dart';

const Platform instance = WebPlatform();

/// [Platform] implementation that delegates to `dart:html`.
/// [Platform] implementation that delegates to `dart:js_interop`.
class WebPlatform extends Platform {
/// Creates a new [Platform].
const WebPlatform();
Expand All @@ -15,11 +16,10 @@ class WebPlatform extends Platform {
String get operatingSystemVersion => 'unknown';

@override
String get localHostname => html.window.location.hostname ?? 'unknown';
String get localHostname => web.window.location.hostname;

String _browserPlatform() {
final navigatorPlatform =
html.window.navigator.platform?.toLowerCase() ?? '';
final navigatorPlatform = web.window.navigator.platform.toLowerCase();
if (navigatorPlatform.startsWith('mac')) {
return 'macos';
}
Expand All @@ -43,7 +43,7 @@ class WebPlatform extends Platform {
// indicates that a device has a "fine pointer" (mouse) as the primary
// pointing device, then we'll assume desktop linux, and otherwise we'll
// assume Android.
if (html.window.matchMedia('only screen and (pointer: fine)').matches) {
if (web.window.matchMedia('only screen and (pointer: fine)').matches) {
return 'linux';
}
return 'android';
Expand Down
2 changes: 1 addition & 1 deletion dart/lib/src/platform/platform.dart
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import '_io_platform.dart' if (dart.library.html) '_web_platform.dart'
import '_io_platform.dart' if (dart.library.js_interop) '_web_platform.dart'
as platform;

const Platform instance = platform.instance;
Expand Down
2 changes: 1 addition & 1 deletion dart/lib/src/sentry_client_stub.dart
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,4 @@ import 'sentry_options.dart';
/// Implemented in `sentry_browser_client.dart` and `sentry_io_client.dart`.
SentryClient createSentryClient(SentryOptions options) =>
throw UnsupportedError(
'Cannot create a client without dart:html or dart:io.');
'Cannot create a client without dart:js_interop or dart:io.');
2 changes: 1 addition & 1 deletion dart/lib/src/sentry_stack_trace_factory.dart
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import 'package:meta/meta.dart';
import 'package:stack_trace/stack_trace.dart';

import 'noop_origin.dart' if (dart.library.html) 'origin.dart';
import 'noop_origin.dart' if (dart.library.js_interop) 'origin.dart';
import 'protocol.dart';
import 'sentry_options.dart';

Expand Down
2 changes: 1 addition & 1 deletion dart/lib/src/utils/isolate_utils.dart
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import 'package:meta/meta.dart';

import '_io_get_isolate_name.dart'
if (dart.library.html) '_web_get_isolate_name.dart' as isolate_getter;
if (dart.library.js_interop) '_web_get_isolate_name.dart' as isolate_getter;

@internal
String? getIsolateName() => isolate_getter.getIsolateName();
1 change: 1 addition & 0 deletions dart/pubspec.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ dependencies:
meta: ^1.3.0
stack_trace: ^1.10.0
uuid: '>=3.0.0 <5.0.0'
web: ^0.5.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we actually need to add this dependency? Tests passed locally regardless.

Suggested change
web: ^0.5.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

package:web is used in the dart package. Removing this raises a bunch of depend_on_referenced_packages analyzer warnings.

Copy link

Choose a reason for hiding this comment

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

Yeah, you have to leave it!

Copy link
Collaborator

Choose a reason for hiding this comment

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

The question is whether it actually works. We can ignore the warning for some time before cutting the compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite understand why we can leave it out since we have to use the package

import 'package:web/web.dart' as web show window, Window, Navigator;

// and in the enricher event processor we use web.window


dev_dependencies:
build_runner: ^2.4.2
Expand Down
4 changes: 2 additions & 2 deletions dart/test/event_processor/enricher/web_enricher_test.dart
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
@TestOn('browser')
library dart_test;

import 'dart:html' as html;
import 'package:web/web.dart' as web;

import 'package:sentry/sentry.dart';
import 'package:sentry/src/event_processor/enricher/web_enricher_event_processor.dart';
Expand Down Expand Up @@ -208,7 +208,7 @@ class Fixture {
checker: MockPlatformChecker(hasNativeIntegration: false));

return WebEnricherEventProcessor(
html.window,
web.window,
options,
);
}
Expand Down
2 changes: 1 addition & 1 deletion dart/test/stack_trace_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import 'package:sentry/sentry.dart';
import 'package:sentry/src/noop_origin.dart'
if (dart.library.html) 'package:sentry/src/origin.dart';
if (dart.library.js_interop) 'package:sentry/src/origin.dart';
import 'package:sentry/src/sentry_stack_trace_factory.dart';
import 'package:stack_trace/stack_trace.dart';
import 'package:test/test.dart';
Expand Down
2 changes: 1 addition & 1 deletion file/lib/src/sentry_file_extension.dart
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// ignore_for_file: invalid_use_of_internal_member

import 'dart:io' if (dart.library.html) 'dart:html';
import 'dart:io' if (dart.library.js_interop) 'dart:js_interop';

import 'package:meta/meta.dart';
import 'package:sentry/sentry.dart';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import 'noop_connectivity_provider.dart'
if (dart.library.html) 'web_connectivity_provider.dart';
if (dart.library.js_interop) 'web_connectivity_provider.dart';

abstract class ConnectivityProvider {
factory ConnectivityProvider() => connectivityProvider();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import 'dart:async';
import 'dart:html' as html;
import 'package:web/web.dart' as web;

import 'connectivity_provider.dart';

Expand All @@ -8,15 +8,19 @@ ConnectivityProvider connectivityProvider() {
}

class WebConnectivityProvider implements ConnectivityProvider {
StreamSubscription<html.Event>? _onOnlineSub;
StreamSubscription<html.Event>? _onOfflineSub;
StreamSubscription<web.Event>? _onOnlineSub;
StreamSubscription<web.Event>? _onOfflineSub;

@override
void listen(void Function(String connectivity) onChange) {
_onOnlineSub = html.window.onOnline.listen((_) {
_onOnlineSub = web.EventStreamProviders.onlineEvent
.forElement(web.document.body!)
.listen((_) {
onChange('wifi');
});
_onOfflineSub = html.window.onOffline.listen((_) {
_onOfflineSub = web.EventStreamProviders.offlineEvent
.forElement(web.document.body!)
.listen((_) {
onChange('none');
});
}
Expand Down
2 changes: 1 addition & 1 deletion flutter/lib/src/native/factory.dart
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export 'factory_real.dart' if (dart.library.html) 'factory_web.dart';
export 'factory_real.dart' if (dart.library.js_interop) 'factory_web.dart';
12 changes: 9 additions & 3 deletions flutter/lib/src/renderer/html_renderer.dart
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import 'dart:js' as js;
import 'dart:js_interop';

import 'renderer.dart';

Expand All @@ -7,6 +7,12 @@ FlutterRenderer? getRenderer() {
}

bool get isCanvasKitRenderer {
final flutterCanvasKit = js.context['flutterCanvasKit'];
return flutterCanvasKit != null;
return _windowFlutterCanvasKit != null;
}

// These values are set by the engine. They are used to determine if the
// application is using canvaskit or skwasm.
//
// See https://github.com/flutter/flutter/blob/414d9238720a3cde85475f49ce0ba313f95046f7/packages/flutter/lib/src/foundation/_capabilities_web.dart#L10
@JS('window.flutterCanvasKit')
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could also add the comment fron the link as decriptions.

// These values are set by the engine. They are used to determine if the
// application is using canvaskit or skwasm.
// 
// See https://github.com/flutter/flutter/blob/414d9238720a3cde85475f49ce0ba313f95046f7/packages/flutter/lib/src/foundation/_capabilities_web.dart#L10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great idea

external JSAny? get _windowFlutterCanvasKit;
Loading
Loading