-
-
Notifications
You must be signed in to change notification settings - Fork 277
Migrates to package:web and js_interop #2064
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
Changes from 1 commit
a9838a9
9a156fb
4266881
7dfc42e
2bf4b4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| 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; | ||
|
|
||
|
|
@@ -42,29 +42,22 @@ class WebEnricherEventProcessor implements EnricherEventProcessor { | |
| // https://github.com/getsentry/sentry-javascript/blob/a6f8dc26a4c7ae2146ae64995a2018c8578896a6/packages/browser/src/integrations/useragent.ts | ||
| SentryRequest _getRequest(SentryRequest? request) { | ||
| final requestHeader = request?.headers; | ||
| final header = requestHeader == null | ||
| ? <String, String>{} | ||
| : Map<String, String>.from(requestHeader); | ||
| final header = requestHeader == null ? <String, String>{} : Map<String, String>.from(requestHeader); | ||
|
|
||
| header.putIfAbsent('User-Agent', () => _window.navigator.userAgent); | ||
|
|
||
| final url = request?.url ?? _window.location.toString(); | ||
| return (request ?? SentryRequest(url: url)) | ||
| .copyWith(headers: header) | ||
| .sanitized(); | ||
| return (request ?? SentryRequest(url: url)).copyWith(headers: header).sanitized(); | ||
| } | ||
|
|
||
| SentryDevice _getDevice(SentryDevice? device) { | ||
| return (device ?? SentryDevice()).copyWith( | ||
| 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(), | ||
| screenDensity: | ||
| device?.screenDensity ?? _window.devicePixelRatio.toDouble(), | ||
| screenHeightPixels: device?.screenHeightPixels ?? _window.screen.availHeight, | ||
| screenWidthPixels: device?.screenWidthPixels ?? _window.screen.availWidth, | ||
| screenDensity: device?.screenDensity ?? _window.devicePixelRatio.toDouble(), | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -77,14 +70,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 +92,7 @@ class WebEnricherEventProcessor implements EnricherEventProcessor { | |
| ); | ||
| } | ||
| } | ||
|
|
||
| extension on web.Navigator { | ||
| external double? get deviceMemory; | ||
|
Collaborator
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.
Contributor
Author
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 | ||||
|
Contributor
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
Contributor
Author
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!
Contributor
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.
Contributor
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,9 @@ FlutterRenderer? getRenderer() { | |
| } | ||
|
|
||
| bool get isCanvasKitRenderer { | ||
| final flutterCanvasKit = js.context['flutterCanvasKit']; | ||
| return flutterCanvasKit != null; | ||
| return _windowFlutterCanvasKit != null; | ||
| } | ||
|
|
||
| // See https://github.com/flutter/flutter/blob/414d9238720a3cde85475f49ce0ba313f95046f7/packages/flutter/lib/src/foundation/_capabilities_web.dart#L10 | ||
|
Collaborator
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
Contributor
Author
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; | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -27,6 +27,7 @@ dependencies: | |||
| package_info_plus: '>=1.0.0' | ||||
| meta: ^1.3.0 | ||||
| ffi: ^2.0.0 | ||||
| web: ^0.5.1 | ||||
|
Contributor
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. same question as above
Suggested change
|
||||
|
|
||||
| dev_dependencies: | ||||
| build_runner: ^2.4.2 | ||||
|
|
||||
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.
is it possible to keep the original code in place for consumers with an older Flutter version? Something like
Uh oh!
There was an error while loading. Please reload this page.
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.
that works and is also done inside the poc
in your scenario example
web_enricher_event_processor_legacy.dartwould use dart:htmland
'web_enricher_event_processor.dartwould 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