SDK-2881 Break Up Static Methods and Remove Error Classes#1345
Conversation
fadi-george
force-pushed
the
fadi/sdk-2871-use-simpler-user-agent-helpers
branch
5 times, most recently
from
747f4a3 to
5463db9
Compare
There was a problem hiding this comment.
nit: is this file necessary? I was under the impression that barrel files can potentially cause issues with tree shaking. Curious if excluding it would have any effect on bundle size.
There was a problem hiding this comment.
Changed to requests/api
| // we may have supported both of these keys in the past (with and without "Text" postfix) | ||
| // so we're leaving here and checking in case it is being used this way | ||
| const higherLevelAcceptButtonText = | ||
| promptOptions.acceptButtonText || promptOptions.acceptButton; | ||
| const higherLevelCancelButtonText = | ||
| promptOptions.cancelButtonText || promptOptions.cancelButton; | ||
|
|
||
| /** | ||
| * we should give preference to the lower level ("slidedown" level) text settings in the case that | ||
| * text settings are configured at the higher level as well as the lower level | ||
| * | ||
| * Example: | ||
| * "promptOptions": { | ||
| * "acceptButtonText": "", | ||
| * "cancelButtonText": "", | ||
| * "slidedown": { | ||
| * "acceptButtonText": "", <-- | ||
| * "cancelButtonText": "" <-- | ||
| * } | ||
| * } | ||
| */ |
There was a problem hiding this comment.
Just my opinion but I think keeping these comments would be useful
There was a problem hiding this comment.
same question re: barrel files
There was a problem hiding this comment.
Removed barrel files.
| throw new OneSignalError( | ||
| 'Login: Consent required but not given, skipping login', | ||
| ); | ||
| throw new Error('Consent required but not given'); |
There was a problem hiding this comment.
Any particular reason for changing the error text itself? imo the effect of the error seemed more clear beforehand
There was a problem hiding this comment.
Having shorter messages would be better for reducing the bundle sizes. So striking a balance would be good.
| new Error(`"${argName}" is out of range`); | ||
|
|
||
| export const WrongTypeArgumentError = (argName: string) => | ||
| new Error(`"${argName}" is the wrong type`); |
There was a problem hiding this comment.
nit: might be nice to throw in a is the wrong type: expected {$expected} but got {$actual}
There was a problem hiding this comment.
I'd expect people to read the docs first and that this error should be rare.
fadi-george
force-pushed
the
fadi/sdk-2871-use-simpler-user-agent-helpers
branch
from
5463db9 to
b069192
Compare
- split service worker helpers - clean up config heleprs - split local storage helpers - clean up notification types
fadi-george
force-pushed
the
fadi/sdk-2881-break-up-more-static-methods-like-requestservice
branch
from
9c4f791 to
4b5f694
Compare
There was a problem hiding this comment.
nit: should this go into general helpers?
fadi-george
deleted the
fadi/sdk-2881-break-up-more-static-methods-like-requestservice
branch
2 participants
Description
1 Line Summary
More optimizations to the bundle size.
Details
Shave off more kBs off the bundles by breaking up static classes helpers like request service.
# SDK-2871 build/releases/OneSignalSDK.page.js Size limit: 631 B Size: 631 B gzipped build/releases/OneSignalSDK.page.es6.js Size limit: 60.6 kB Size: 60.53 kB gzipped build/releases/OneSignalSDK.sw.js Size limit: 25.25 kB Size: 25.19 kB gzipped build/releases/OneSignalSDK.page.styles.css Size limit: 8.81 kB Size: 8.8 kB gzippedNow:
Systems Affected
Validation
Tests
Info
Checklist
Programming Checklist
Interfaces:
Functions:
Typescript:
Other:
elem of arraysyntax. PreferforEachor usemapcontextif possible. Instead, we can pass it to function/constructor so that we don't callOneSignal.contextScreenshots
Info
Checklist
Related Tickets
This change is