-
-
Notifications
You must be signed in to change notification settings - Fork 238
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
Changes from all commits
a9838a9
9a156fb
4266881
7dfc42e
2bf4b4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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" | ||||||
|
||||||
- name: Build Android | ||||||
run: | | ||||||
|
@@ -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" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
- name: Build iOS | ||||||
run: | | ||||||
|
@@ -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" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
- name: Build web | ||||||
run: | | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
|
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'; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 and 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) => | ||||||||
|
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, | ||
); | ||
} | ||
|
@@ -16,7 +16,7 @@ class WebEnricherEventProcessor implements EnricherEventProcessor { | |
this._options, | ||
); | ||
|
||
final html.Window _window; | ||
final web.Window _window; | ||
|
||
final SentryOptions _options; | ||
|
||
|
@@ -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(), | ||
); | ||
|
@@ -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; | ||
} | ||
|
@@ -101,3 +98,7 @@ class WebEnricherEventProcessor implements EnricherEventProcessor { | |
); | ||
} | ||
} | ||
|
||
extension on web.Navigator { | ||
external double? get deviceMemory; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this used anywhere? If not, we can remove it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's currently used in the |
||
} |
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}/'; |
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(); |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -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 | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, you have to leave it! There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||
|
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'; |
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'; | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. great idea |
||
@JS('window.flutterCanvasKit') | ||
external JSAny? get _windowFlutterCanvasKit; |
There was a problem hiding this comment.
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
: