-
-
Notifications
You must be signed in to change notification settings - Fork 344
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
feat(feedback): Support web environments #4558
Conversation
# Conflicts: # packages/core/src/js/feedback/FeedbackWidget.tsx
…-screenshot # Conflicts: # packages/core/src/js/feedback/FeedbackWidget.tsx
…-simplify-onaddscreenshot # Conflicts: # packages/core/src/js/feedback/FeedbackWidget.tsx
…-simplify-onaddscreenshot
|
Android (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9402883 | 448.53 ms | 468.73 ms | 20.20 ms |
e42816c | 401.30 ms | 410.04 ms | 8.74 ms |
3e4cdf5 | 462.35 ms | 474.96 ms | 12.61 ms |
77e88fc | 478.48 ms | 487.21 ms | 8.73 ms |
8cb898b | 438.83 ms | 420.58 ms | -18.25 ms |
2646c98 | 429.98 ms | 421.63 ms | -8.35 ms |
6b1624f | 462.78 ms | 465.13 ms | 2.35 ms |
df05370 | 477.62 ms | 491.63 ms | 14.00 ms |
e5d5735 | 452.70 ms | 453.04 ms | 0.34 ms |
894ebb0 | 497.45 ms | 545.04 ms | 47.60 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9402883 | 17.75 MiB | 20.12 MiB | 2.37 MiB |
e42816c | 17.75 MiB | 20.12 MiB | 2.38 MiB |
3e4cdf5 | 17.75 MiB | 20.12 MiB | 2.37 MiB |
77e88fc | 17.75 MiB | 20.12 MiB | 2.37 MiB |
8cb898b | 17.75 MiB | 20.12 MiB | 2.37 MiB |
2646c98 | 17.75 MiB | 20.12 MiB | 2.37 MiB |
6b1624f | 17.75 MiB | 20.12 MiB | 2.37 MiB |
df05370 | 17.75 MiB | 20.12 MiB | 2.37 MiB |
e5d5735 | 17.75 MiB | 20.12 MiB | 2.37 MiB |
894ebb0 | 17.75 MiB | 20.12 MiB | 2.37 MiB |
iOS (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
e5d5735+dirty | 1222.02 ms | 1222.22 ms | 0.20 ms |
8cb898b+dirty | 1221.40 ms | 1231.78 ms | 10.37 ms |
77e88fc+dirty | 1224.55 ms | 1232.41 ms | 7.86 ms |
9402883+dirty | 1219.65 ms | 1217.94 ms | -1.72 ms |
894ebb0+dirty | 1224.33 ms | 1214.45 ms | -9.89 ms |
e42816c+dirty | 1211.29 ms | 1219.65 ms | 8.37 ms |
0325426+dirty | 1228.88 ms | 1229.92 ms | 1.04 ms |
0459aee+dirty | 1232.82 ms | 1231.19 ms | -1.63 ms |
6b1624f+dirty | 1224.65 ms | 1225.65 ms | 1.00 ms |
269c976+dirty | 1210.02 ms | 1204.46 ms | -5.56 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
e5d5735+dirty | 2.63 MiB | 3.69 MiB | 1.06 MiB |
8cb898b+dirty | 2.63 MiB | 3.71 MiB | 1.08 MiB |
77e88fc+dirty | 2.63 MiB | 3.75 MiB | 1.12 MiB |
9402883+dirty | 2.63 MiB | 3.71 MiB | 1.07 MiB |
894ebb0+dirty | 2.63 MiB | 3.71 MiB | 1.07 MiB |
e42816c+dirty | 2.63 MiB | 3.75 MiB | 1.12 MiB |
0325426+dirty | 2.63 MiB | 3.69 MiB | 1.06 MiB |
0459aee+dirty | 2.63 MiB | 3.69 MiB | 1.06 MiB |
6b1624f+dirty | 2.63 MiB | 3.71 MiB | 1.07 MiB |
269c976+dirty | 2.63 MiB | 3.69 MiB | 1.06 MiB |
Previous results on branch: antonis/feedback-webfixes
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d7f56e5+dirty | 1215.96 ms | 1222.83 ms | 6.87 ms |
e9a7e86+dirty | 1219.31 ms | 1223.33 ms | 4.01 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d7f56e5+dirty | 2.63 MiB | 3.76 MiB | 1.12 MiB |
e9a7e86+dirty | 2.63 MiB | 3.76 MiB | 1.12 MiB |
iOS (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
e5d5735+dirty | 1217.78 ms | 1221.80 ms | 4.02 ms |
8cb898b+dirty | 1209.39 ms | 1207.57 ms | -1.82 ms |
77e88fc+dirty | 1218.79 ms | 1220.00 ms | 1.21 ms |
9402883+dirty | 1217.71 ms | 1213.02 ms | -4.69 ms |
894ebb0+dirty | 1210.94 ms | 1202.08 ms | -8.85 ms |
e42816c+dirty | 1220.08 ms | 1222.46 ms | 2.38 ms |
0325426+dirty | 1210.17 ms | 1216.37 ms | 6.20 ms |
0459aee+dirty | 1233.67 ms | 1239.80 ms | 6.12 ms |
6b1624f+dirty | 1224.12 ms | 1220.73 ms | -3.39 ms |
269c976+dirty | 1223.29 ms | 1222.90 ms | -0.39 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
e5d5735+dirty | 3.19 MiB | 4.26 MiB | 1.07 MiB |
8cb898b+dirty | 3.19 MiB | 4.28 MiB | 1.09 MiB |
77e88fc+dirty | 3.19 MiB | 4.32 MiB | 1.13 MiB |
9402883+dirty | 3.19 MiB | 4.27 MiB | 1.09 MiB |
894ebb0+dirty | 3.19 MiB | 4.27 MiB | 1.09 MiB |
e42816c+dirty | 3.19 MiB | 4.32 MiB | 1.13 MiB |
0325426+dirty | 3.19 MiB | 4.26 MiB | 1.07 MiB |
0459aee+dirty | 3.19 MiB | 4.26 MiB | 1.07 MiB |
6b1624f+dirty | 3.19 MiB | 4.27 MiB | 1.09 MiB |
269c976+dirty | 3.19 MiB | 4.26 MiB | 1.07 MiB |
Previous results on branch: antonis/feedback-webfixes
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d7f56e5+dirty | 1236.69 ms | 1236.24 ms | -0.45 ms |
e9a7e86+dirty | 1225.75 ms | 1219.46 ms | -6.29 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d7f56e5+dirty | 3.19 MiB | 4.32 MiB | 1.14 MiB |
e9a7e86+dirty | 3.19 MiB | 4.32 MiB | 1.14 MiB |
|
||
export const feedbackAlertDialog = (title: string, message: string): void => { | ||
/* eslint-disable @typescript-eslint/ban-ts-comment, no-restricted-globals, no-alert, @typescript-eslint/no-unsafe-member-access */ | ||
// @ts-ignore |
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 used ts-ignore
to avoid touching the global state and compilerOptions to use window
. I'll be happy to iterate if there is better suggestion for this 🙇
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.
One thing you could try is altering the worldwide.ts file at packages/core/src/js/utils/worldwride.ts
Change the ReactNativeInternalGlobal and add the following parameter:
alert?: (message: string) => void;
So with that you could use the following code
if (isWeb() && typeof RN_GLOBAL_OBJ.alert !== 'undefined') {
RN_GLOBAL_OBJ.alert(`${title}\n${message}`);
} else {
EDIT: I think there may be an issue with my approach, RN_GLOBAL_OBJ
points to globalThis
and the alert is inside globalThis.window
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 the suggestion @lucas-zimerman 🙇
This actually seems to work pretty well and there is no need to suppress any typescript checks.
Updated with 28738bf
Android (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9402883+dirty | 436.49 ms | 518.94 ms | 82.45 ms |
8cb898b+dirty | 393.33 ms | 416.20 ms | 22.87 ms |
6b1624f+dirty | 382.17 ms | 441.00 ms | 58.83 ms |
2646c98+dirty | 415.13 ms | 438.41 ms | 23.28 ms |
3e4cdf5+dirty | 642.13 ms | 702.23 ms | 60.10 ms |
0459aee+dirty | 424.10 ms | 466.63 ms | 42.53 ms |
b74349e+dirty | 349.96 ms | 375.00 ms | 25.04 ms |
e42816c+dirty | 347.06 ms | 348.18 ms | 1.12 ms |
df05370+dirty | 395.08 ms | 430.38 ms | 35.30 ms |
d1a10a1+dirty | 364.43 ms | 362.98 ms | -1.45 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9402883+dirty | 7.15 MiB | 8.39 MiB | 1.23 MiB |
8cb898b+dirty | 7.15 MiB | 8.39 MiB | 1.24 MiB |
6b1624f+dirty | 7.15 MiB | 8.39 MiB | 1.23 MiB |
2646c98+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
3e4cdf5+dirty | 7.15 MiB | 8.39 MiB | 1.23 MiB |
0459aee+dirty | 7.15 MiB | 8.38 MiB | 1.23 MiB |
b74349e+dirty | 7.15 MiB | 8.39 MiB | 1.24 MiB |
e42816c+dirty | 7.15 MiB | 8.39 MiB | 1.24 MiB |
df05370+dirty | 7.15 MiB | 8.39 MiB | 1.23 MiB |
d1a10a1+dirty | 7.15 MiB | 8.39 MiB | 1.24 MiB |
Previous results on branch: antonis/feedback-webfixes
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d7f56e5+dirty | 391.04 ms | 397.85 ms | 6.81 ms |
e9a7e86+dirty | 407.96 ms | 420.29 ms | 12.33 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d7f56e5+dirty | 7.15 MiB | 8.39 MiB | 1.24 MiB |
e9a7e86+dirty | 7.15 MiB | 8.39 MiB | 1.24 MiB |
Co-authored-by: Krystof Woldrich <[email protected]>
…-simplify-onaddscreenshot
…/feedback-webfixes
# Conflicts: # packages/core/src/js/feedback/FeedbackWidget.tsx
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! Works as expected with Expo Web.
📢 Type of change
Based on #4546 to avoid conflicts
📜 Description
This PR handles the following issues on the web:
window.alert
💡 Motivation and Context
See #4302 (comment)
💚 How did you test it?
Manual
📝 Checklist
sendDefaultPII
is enabled🔮 Next steps
#skip-changelog