POC: Move `package:web` access to separate package #2110

denrase · 2024-06-18T13:45:08Z

📜 Description

Evaluate if we can move package:web access to it's own library

💡 Motivation and Context

Provide a path to support compilation via WASM without bumping Dart/Flutter SDKs in main sentry package

Relates to #2104

💚 How did you test it?

📝 Checklist

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

🔮 Next steps

github-actions · 2024-06-18T13:46:02Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Move `package:web` access to separate package ([#2110](https://github.com/getsentry/sentry-dart/pull/2110))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against bd8e02d

denrase · 2024-06-18T13:47:23Z

dart/lib/src/sentry.dart


    try {
      final config = optionsConfiguration(sentryOptions);
      if (config is Future) {
        await config;
      }
+      await _initDefaultValues(sentryOptions);


Changing this order probably breaks stuff in unpredictable ways...

Why do we have to change the ordering?

Ok nvm I understand.

We'll have to think further about this. Like you said this might break stuff / have side effects

We can have a static setWindow() function which sets an internal window and then assign that to options when calling init.

Window _window = NoopWindow(); void setWindow(Window window) { if (!PlatformChecker().isWeb) { return; } _window = window; } // and then inside init() final sentryOptions = options ?? SentryOptions(); options?.window = _window;

The user just needs to call the setWindow function before SentryFlutter.init if they wanna do wasm

Sentry.setWindow(...) SentryFlutter.init(...)

at least that way we can keep the ordering. maybe there's better ways

This is still a breaking change, isn't it? As in, things that worked before for users won't work anymore unless they change their code.

This is still a breaking change, isn't it? As in, things that worked before for users won't work anymore unless they change their code.

tbh I'm not sure, wasm compilation only works on the latest stable channel so they all have to upgrade flutter afaik and it's not working right now for them anyway.

non-wasm users won't have to care about this stuff

What I meant is that it would be breaking for existing users targeting web? Or am I missing something.

codecov · 2024-06-18T13:48:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.25%. Comparing base (c3b8a98) to head (bd8e02d).
Report is 4 commits behind head on main.

❗ Current head bd8e02d differs from pull request most recent head 5c0480b

Please upload reports for the commit 5c0480b to get more accurate results.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2110   +/-   ##
=======================================
  Coverage   95.25%   95.25%           
=======================================
  Files          54       54           
  Lines        1791     1791           
=======================================
  Hits         1706     1706           
  Misses         85       85

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-06-18T13:52:15Z

Android Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	373.73 ms	468.98 ms	95.25 ms
Size	6.35 MiB	7.34 MiB	1008.27 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`e893df5`	310.60 ms	380.58 ms	69.98 ms
`6aab859`	306.27 ms	377.92 ms	71.65 ms
`50bdfad`	395.22 ms	461.21 ms	65.98 ms
`5cc82a0`	400.35 ms	474.52 ms	74.17 ms
`0f067d3`	359.56 ms	431.28 ms	71.72 ms
`1a93825`	347.31 ms	424.54 ms	77.23 ms
`b8562d0`	362.31 ms	437.27 ms	74.96 ms
`3e5ee37`	317.56 ms	366.84 ms	49.28 ms
`3a69405`	334.34 ms	369.19 ms	34.85 ms
`379d7a8`	327.10 ms	355.39 ms	28.29 ms

App size

Revision	Plain	With Sentry	Diff
`e893df5`	6.06 MiB	7.09 MiB	1.03 MiB
`6aab859`	6.15 MiB	7.13 MiB	999.97 KiB
`50bdfad`	6.33 MiB	7.30 MiB	987.47 KiB
`5cc82a0`	6.34 MiB	7.28 MiB	966.66 KiB
`0f067d3`	6.33 MiB	7.30 MiB	992.08 KiB
`1a93825`	6.27 MiB	7.20 MiB	956.36 KiB
`b8562d0`	6.35 MiB	7.33 MiB	1005.53 KiB
`3e5ee37`	5.94 MiB	6.92 MiB	1001.19 KiB
`3a69405`	5.94 MiB	6.95 MiB	1.01 MiB
`379d7a8`	5.94 MiB	6.95 MiB	1.01 MiB

denrase · 2024-06-18T16:00:36Z

@buenaflor This might just work, i build with WebAssembly and triggered an issue. Before, I set some sample values in the separate package.

Dummy values in package:

Code

Removed all dependencies that hindered building for WebAssembly in the sample app. Removed a flutter event processor so that the web values would be taken as fallback.

flutter build web --wasm
flutter pub global activate dhttpd
cd build/web
dart pub global run dhttpd '--headers=Cross-Origin-Embedder-Policy=credentialless;Cross-Origin-Opener-Policy=same-origin'

Result

https://sentry-sdks.sentry.io/issues/5503776648/?project=5428562&query=is%3Aunresolved+issue.priority%3A%5Bhigh%2C+medium%5D&referrer=issue-stream&statsPeriod=1h&stream_index=0

buenaflor · 2024-06-19T19:07:50Z

did some quick tests and works well on dart 3.2.0 and flutter 16.0

buenaflor · 2024-06-19T22:26:03Z

dart/lib/src/web/web_window.dart

+
+// Get window from options or noop
+Window createWindow(SentryOptions options) {
+  return options.window() ?? NoopWindow();


We create the versions.dart that contains the package version and name and add it to the options like here

since we don't use the options inside the sentry_web package, maybe we can set it here?

technically the user could provide their own Window implementation but I don't think it's likely that they do that

buenaflor · 2024-06-19T22:47:18Z

dart/lib/src/sentry.dart


    try {
      final config = optionsConfiguration(sentryOptions);
      if (config is Future) {
        await config;
      }
+      await _initDefaultValues(sentryOptions);


Ok nvm I understand.

We'll have to think further about this. Like you said this might break stuff / have side effects

buenaflor · 2024-06-19T22:56:39Z

dart/lib/src/sentry.dart


    try {
      final config = optionsConfiguration(sentryOptions);
      if (config is Future) {
        await config;
      }
+      await _initDefaultValues(sentryOptions);


We can have a static setWindow() function which sets an internal window and then assign that to options when calling init.

Window _window = NoopWindow(); void setWindow(Window window) { if (!PlatformChecker().isWeb) { return; } _window = window; } // and then inside init() final sentryOptions = options ?? SentryOptions(); options?.window = _window;

The user just needs to call the setWindow function before SentryFlutter.init if they wanna do wasm

Sentry.setWindow(...) SentryFlutter.init(...)

at least that way we can keep the ordering. maybe there's better ways

denrase added 2 commits June 18, 2024 15:41

external web package sample

c7ecaca

Merge branch 'main' into test/web-package-in-external-lib

e0c9796

denrase commented Jun 18, 2024

View reviewed changes

lower dart sdk

016ffc1

comment out all dependencies that hinder wasm build

bd8e02d

buenaflor mentioned this pull request Jun 19, 2024

Migrates to package:web and js_interop #2064

Closed

6 tasks

buenaflor added 5 commits June 20, 2024 00:05

Update sentry version to 8.3.0 in web pubspec.yaml

7da63f0

Remove import in sentry_web.dart

55adc7c

Add changelog symlink

c8d7033

Add dartdoc options symlink

de51ed6

Run dart formatting

5c0480b

buenaflor reviewed Jun 19, 2024

View reviewed changes

vaind mentioned this pull request Jun 20, 2024

feat: enable wasm compilation #2113

Merged

vaind closed this in #2113 Jun 25, 2024

denrase deleted the test/web-package-in-external-lib branch June 25, 2024 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POC: Move `package:web` access to separate package #2110

POC: Move `package:web` access to separate package #2110

denrase commented Jun 18, 2024

github-actions bot commented Jun 18, 2024 •

edited

Loading

denrase Jun 18, 2024 •

edited

Loading

buenaflor Jun 19, 2024

buenaflor Jun 19, 2024

buenaflor Jun 19, 2024 •

edited

Loading

vaind Jun 20, 2024

buenaflor Jun 20, 2024 •

edited

Loading

vaind Jun 20, 2024

codecov bot commented Jun 18, 2024 •

edited

Loading

github-actions bot commented Jun 18, 2024 •

edited

Loading

Baseline results on branch: main

Startup times

App size

denrase commented Jun 18, 2024

buenaflor commented Jun 19, 2024

buenaflor Jun 19, 2024

buenaflor Jun 19, 2024

buenaflor Jun 19, 2024 •

edited

Loading

POC: Move package:web access to separate package #2110

POC: Move package:web access to separate package #2110

Conversation

denrase commented Jun 18, 2024

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

github-actions bot commented Jun 18, 2024 • edited Loading

Instructions and example for changelog

denrase Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

buenaflor Jun 19, 2024

Choose a reason for hiding this comment

buenaflor Jun 19, 2024

Choose a reason for hiding this comment

buenaflor Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

vaind Jun 20, 2024

Choose a reason for hiding this comment

buenaflor Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

vaind Jun 20, 2024

Choose a reason for hiding this comment

codecov bot commented Jun 18, 2024 • edited Loading

Codecov Report

github-actions bot commented Jun 18, 2024 • edited Loading

Android Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

denrase commented Jun 18, 2024

buenaflor commented Jun 19, 2024

buenaflor Jun 19, 2024

Choose a reason for hiding this comment

buenaflor Jun 19, 2024

Choose a reason for hiding this comment

buenaflor Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

POC: Move `package:web` access to separate package #2110

POC: Move `package:web` access to separate package #2110

github-actions bot commented Jun 18, 2024 •

edited

Loading

denrase Jun 18, 2024 •

edited

Loading

buenaflor Jun 19, 2024 •

edited

Loading

buenaflor Jun 20, 2024 •

edited

Loading

codecov bot commented Jun 18, 2024 •

edited

Loading

github-actions bot commented Jun 18, 2024 •

edited

Loading

buenaflor Jun 19, 2024 •

edited

Loading