-
-
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
Conversation
hey, I see that this is for wasm compilation but I don't think we support symbolication for wasm yet |
This is blocking some customers. Happy to discuss how we can help! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2064 +/- ##
==========================================
- Coverage 95.25% 88.64% -6.61%
==========================================
Files 54 223 +169
Lines 1791 7583 +5792
==========================================
+ Hits 1706 6722 +5016
- Misses 85 861 +776 ☔ View full report in Codecov by Sentry. |
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.
Thank you for contributing! I agree that we should migrate to this and also maybe release this in a major version bump.
Some small nitpicks & questions. Also, we need to satisfy CI in order to proceed.
- We are missing a changelog entry
- integration tests on macOS are failing apparently
- min-version tests are failing. Do we need to bump dart/flutter versions?
- analyze is failing.
dart format .
needs to be run & changes committed
@@ -101,3 +92,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 comment
The 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 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.
} | ||
|
||
// 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 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
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.
great idea
quick fyi: the ci job |
@denrase @buenaflor I've addressed the PR comments and hopefully fixed the build failures. I needed to bump the min version of Flutter has Dart 3.1.0 is required by package:web. |
@josh-burton if it's okay with you we'd like to fork your branch and take over from here, it's easier for us to fix the rest of the ci since we don't need to approve runnning them every time (cc @denrase) |
No problem, go for it. |
Any help we can get here? |
hey we've been looking into it. The main problem we have is the necessity to bump the min versions. as far as I can see we would need to bump it to dart 3.3.0 and flutter 3.19.0 which is pretty much a huge jump for us Any recommendations to alleviate that? |
|
We cannot simply bump to versions that are only a few months old, we want to avoid that even in a new major release. A large part of our customers are not on dart 3.3.0 and flutter 3.19 yet (which are very close to the latest versions) and we don't plan on maintaining two majors for an extended period of time. @kevmoo do you have guidance here if we have any other option available other than bumping the min versions? |
@buenaflor – this is a tricky spot. If you want to maintain backward compat, you could test for the existence of I worry that you'll get folks importing this library before it's stable on versions before Dart 3.3 – but it might be work investigating |
@kevmoo We do this, however this does not preclude us from adding the web package to our dependencies, which automatically raises the dart min SDK version to 3.3. Or am I missing something here? |
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.
I've given this a brief look and have some ideas that may be able to unblock this while keeping backward compatibility. I may be missing something but it may be worth giving it a try.
@@ -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 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.
web: ^0.5.1 |
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.
package:web is used in the dart package. Removing this raises a bunch of depend_on_referenced_packages
analyzer warnings.
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.
Yeah, you have to leave it!
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.
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 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
@@ -27,6 +27,7 @@ dependencies: | |||
package_info_plus: '>=1.0.0' | |||
meta: ^1.3.0 | |||
ffi: ^2.0.0 | |||
web: ^0.5.1 |
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.
same question as above
web: ^0.5.1 |
@@ -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" |
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
:
flutter-version: "3.13.0" | |
flutter-version: "3.0.0" |
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
flutter-version: "3.13.0" | |
flutter-version: "3.0.0" |
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
flutter-version: "3.13.0" | |
flutter-version: "3.0.0" |
@@ -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 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
@@ -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 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
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'; |
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.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
@vaind idea might work. I worry about the fact that So we might be in the same spot! |
we have a poc for moving the web package implementations to another package called which then inside the dart library we import it conditionally import '../../web/noop_window.dart'
if (dart.library.html) '../../web/http_window.dart'
if (dart.library.js_interop) '../../web/web_window.dart'; inside a user's application if they wanna be able to build for wasm: import 'package:sentry_web/sentry_web.dart' as sentry_web;
// set the window in SentryFlutter.init
options.window = sentry_web.SentryWeb.createWindow; as far as testing goes it looks fine, doesn't break backwards compat on lower dart versions (🤞). might be a wrong direction though, it was only briefly tested |
Great. That sounds like the best approach. Should I close this PR? |
Having custom code for one platform is not ideal because then people need to know they need to change this, i.e. there need to be docs and they have to notice it in the docs. Just to double check: has anyone tried if my proposals actually work or did we just assume they won't? Because in that case I would give it a try myself. |
We've already decided that we won't bump the versions not matter what for this use case so this solution is the "worst" case scenario if we can't manage anything more simpler.
The approach is very similar to the poc. |
AFAIU it will work because the consumer app depends on the web package. If it doesn't then our code doesn't use it anyway (it should be tree-shaken/trimmed) |
let me check this out in a new PR |
@kevmoo, @srujzs, and I were brainstorming a bit here to see what other alternative we could offer from a Dart perspective, and we landed on either branching (having 2 versions of the package under maintenance) or practically what @buenaflor suggested above (2 versions of the web-specific code, one based on There are a lot of caveats to keep in mind:
Hope this is helpful. |
Thanks for inputs. I was able to make this work today by dropping the import & using conditional imports (i.e. what I've suggested above). See followup PR #2113 - with a little bit of fiddling it seems to work fine in my tests and I don't think it would break anyone currently using the SDK while also enabling people to start using WASM (once we update stack trace parsing & implement offline symbolication for wasm - #1480)
that's right, that's exactly what I did
I'm testing on Dart 3.3+ with the new |
Unfortunately the release of the fix is currently blocked, we've got package validations error when trying to publish:
we're looking into it |
Ah, it makes sense that pub validates that imports correspond to available dependencies. This validates my earlier concern around how to drop the For example: dart/lib/src/origin_web.dart: import 'package:web/web.dart';
/// request origin, used for browser stacktrace
String get eventOrigin => '${window.location.origin}/'; would become: import 'dart:js_interop';
/// request origin, used for browser stacktrace
@JS('window.location.origin')
external String get eventOrigin; OR something like: import 'dart:js_interop';
import 'dart:js_interop_unsafe';
/// request origin, used for browser stacktrace
String get eventOrigin {
final location = globalContext['location'] as JSObject;
final origin = location['origin'] as JSString;
return '${origin.toDart}/';
} And _web_platform.dart can directly expose the two members it uses: import 'dart:js_interop';
import 'dart:js_interop_unsafe';
/// request origin, used for browser stacktrace
String get hostname {
final location = globalContext['location'] as JSObject;
final hostname = location['hostname'] as JSString;
return hostname.toDart;
}
bool matchMediaMatches(String s) {
final result = globalContext.callMethod('matchMedia', s.toJS) as JSObject;
return (result['matches'] as JSBoolean).toDart;
} I believe |
8.4.0-beta.1 is out with WASM support. Can you give it a try @josh-burton, @kevmoo ? |
Thank you so much! |
📜 Description
Migrates to package:web and js_interop
💡 Motivation and Context
Supports compilation via WASM
💚 How did you test it?
Tested on a flutter web app project, ran unit tests
📝 Checklist
sendDefaultPii
is enabled🔮 Next steps
Version numbers should be bumped. I think technically this is not a breaking change, but perhaps it should be treated as one.