Migrates to package:web and js_interop

josh-burton commented 1 month ago

:scroll: Description

:bulb: Motivation and Context

Supports compilation via WASM

:green_heart: How did you test it?

Tested on a flutter web app project, ran unit tests

:pencil: Checklist

[x] I reviewed submitted code
[x] I added tests to verify changes
[x] No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
[x] I updated the docs if needed
[x] All tests passing
[ ] No breaking changes

:crystal_ball: Next steps

Version numbers should be bumped. I think technically this is not a breaking change, but perhaps it should be treated as one.

buenaflor commented 1 month ago

hey, I see that this is for wasm compilation but I don't think we support symbolication for wasm yet

kevmoo commented 3 weeks ago

This is blocking some customers. Happy to discuss how we can help!

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 97.05882% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.64%. Comparing base (e6b16cd) to head (80a0e2b). Report is 21 commits behind head on main.

:exclamation: Current head 80a0e2b differs from pull request most recent head 2bf4b4f

Please upload reports for the commit 2bf4b4f to get more accurate results.

Files	Patch %	Lines
...tter/lib/src/native/cocoa/sentry_native_cocoa.dart	60.00%	2 Missing :warning:
...src/integrations/native_app_start_integration.dart	75.00%	1 Missing :warning:
flutter/lib/src/native/factory_real.dart	85.71%	1 Missing :warning:

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

buenaflor commented 3 weeks ago

quick fyi: the ci jobsentry-sqflite / analyze / package-analysis currently fails expectedly so no need to try and fix this here

josh-burton commented 3 weeks ago

@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.

buenaflor commented 3 weeks ago

@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)

josh-burton commented 2 weeks ago

@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.

kevmoo commented 1 week ago

Any help we can get here?

buenaflor commented 1 week ago

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?

zs-dima commented 1 week ago

@buenaflor

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?

buenaflor commented 1 week ago

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?

kevmoo commented 1 week ago

@buenaflor – this is a tricky spot.

If you want to maintain backward compat, you could test for the existence of dart:js_interop – see https://dart.dev/guides/libraries/create-packages#conditionally-importing-and-exporting-library-files

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

denrase commented 1 week ago

@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?

kevmoo commented 1 week ago

@vaind idea might work. I worry about the fact that dart:js_interop was in Dart 3.2 – just not "stable" - https://api.dart.dev/stable/3.2.1/index.html

So we might be in the same spot!

buenaflor commented 6 days ago

we have a poc for moving the web package implementations to another package called sentry_web

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

josh-burton commented 6 days ago

we have a poc for moving the web package implementations to another package called sentry_web

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?

vaind commented 6 days ago

we have a poc for moving the web package implementations to another package called sentry_web

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

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.

buenaflor commented 6 days ago

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.

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.

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.

The approach is very similar to the poc. I'm only confused about the removal of the web package since we have to use a specific property of that package (window) so I'm not sure how it's possible to leave it out.

vaind commented 6 days ago

I'm only confused about the removal of the web package since we have to use a specific property of that package (window) so I'm not sure how it's possible to leave it out.

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)

vaind commented 6 days ago

let me check this out in a new PR

sigmundch commented 6 days ago

@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 dart:html, the other on dart:js_interop, but a package version.)

There are a lot of caveats to keep in mind:

Import order: the order of the conditional imports matters, I'd recommend dart.library.html to go first. Technically that will keep all JavaScript clients on the old logic for a lot longer (until you are ready to bump this package's SDK constraint and directly use package:web). However, I'm inclined to go in that direction because of the risks of inconsistent behavior with using dart:js_interop depending on the SDK version. If you have a solution that works across all SDK versions consistently, then by all means put dart.library.js_interop first.
dart:js_interop breaking changes: dart:js_interop has existed since Dart 3.1.0 and has gone through several breaking changes until it graduated as a stable library in Dart 3.3.0. If it's possible to make the logic consistent across all versions of dart:js_interop, then great! If not, I believe putting it second in the import order may be good enough. That's because dart2wasm only reached stable in Dart 3.4, so we should expect some consistency to follow from the fact that users are likely only using the newer versions of the SDK.
no extension types: Note that extension types were only added in Dart 3.3, which is what we default to use now for JS-interop APIs. To use dart:js_interop without a min-SDK constraint, you will likely need to use older forms of interop. This may be to use @staticInterop or rely on the more primitive parts of the API (like dart:js_interop_unsafe).
dropping package:web: I think this is OK to do, but I wouldn't rely on that dependency existing on user's package. Instead, I would suggest you simply don't use the package altogether. The good news is that all of package:web is written using dart:js_interop, so it is possible to access directly the browser APIs you need using only the Dart/JS interop layer (e.g. use JS() external JSObject get window; to expose window directly, if you need it)

Hope this is helpful.

vaind commented 5 days ago

@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 dart:html, the other on dart:js_interop, but a package version.)

There are a lot of caveats to keep in mind:

Import order: the order of the conditional imports matters, I'd recommend dart.library.html to go first. Technically that will keep all JavaScript clients on the old logic for a lot longer (until you are ready to bump this package's SDK constraint and directly use package:web). However, I'm inclined to go in that direction because of the risks of inconsistent behavior with using dart:js_interop depending on the SDK version. If you have a solution that works across all SDK versions consistently, then by all means put dart.library.js_interop first.

_dart:js_interop breaking changes_: dart:js_interop has existed since Dart 3.1.0 and has gone through several breaking changes until it graduated as a stable library in Dart 3.3.0. If it's possible to make the logic consistent across all versions of dart:js_interop, then great! If not, I believe putting it second in the import order may be good enough. That's because dart2wasm only reached stable in Dart 3.4, so we should expect some consistency to follow from the fact that users are likely only using the newer versions of the SDK.

no extension types: Note that extension types were only added in Dart 3.3, which is what we default to use now for JS-interop APIs. To use dart:js_interop without a min-SDK constraint, you will likely need to use older forms of interop. This may be to use @staticInterop or rely on the more primitive parts of the API (like dart:js_interop_unsafe).

dropping package:web: I think this is OK to do, but I wouldn't rely on that dependency existing on user's package. Instead, I would suggest you simply don't use the package altogether. The good news is that all of package:web is written using dart:js_interop, so it is possible to access directly the browser APIs you need using only the Dart/JS interop layer (e.g. use JS() external JSObject get window; to expose window directly, if you need it)

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 https://github.com/getsentry/sentry-dart/pull/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)

Import order: the order of the conditional imports matters, I'd recommend dart.library.html to go first

that's right, that's exactly what I did

_dart:js_interop breaking changes_: dart:js_interop has existed since Dart 3.1.0 and has gone through several breaking changes until it graduated as a stable library in Dart 3.3.0

I'm testing on Dart 3.3+ with the new package:web dependency and it seems to work for me.

getsentry / sentry-dart