Skip to content
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: OneSignal.setSWLogging; improved testability #999

Conversation

onethirtyfive
Copy link

@onethirtyfive onethirtyfive commented Mar 4, 2023

Description

One Line Summary

Adds inability to turn on/off SW logging from page without directly using internals (#992).

Details

This PR:

  • has minimal impact on internal API contracts
  • introduces SWLog, previously sharing the name of Page Log but not its implementation. SWLog:
    • follows the singleton pattern used elsewhere in repo
    • has good tests
    • SWLog is now unambiguous vs Page Log use--lower cognitive overhead
  • Added OneSignal.setSWLogging(enabled: boolean):
    • small refactor extracts WorkerMessengerCommandHandlers from ServiceWorker for testing surface area, used in this PR
    • uses existing workerMessenger API to send a SetLogging message to SW
    • workerMessenger now logs whether message receipt enables or disables SWLog

Because Service Workers are persistent, SW logging is unaffected by page refresh. This isn't new, but it behaves like setLevel (which is persisted in Database). Happy coincidence. :)

Associated documentation would need updating.

Systems Affected

  • WebSDK
  • Backend
  • Dashboard

Validation

See screenshots.

Tests

  • thorough unit testing of SWLog now possible with singleton pattern
  • small refactor enabled good SetLogging message handling tests

Checklist

  • All the automated tests pass or I explained why that is not possible
  • I have personally tested this on my machine or explained why that is not possible
  • I have included test coverage for these changes or explained why they are not needed

Programming Checklist
Interfaces:

  • Don't use default export
    note: I did leave it in place on SWLog
  • New interfaces are in model files
    please confirm interfaces are in right place

Functions:

  • Don't use default export
  • All function signatures have return types
  • Helpers should not access any data but rather be given the data to operate on.

Typescript:

  • No Typescript warnings
  • Avoid silencing null/undefined warnings with the exclamation point

Other:

  • Iteration: refrain from using elem of array syntax. Prefer forEach or use map
  • Avoid using global OneSignal accessor for context if possible. Instead, we can pass it to function/constructor so that we don't call OneSignal.context

Screenshots

Note that workerMessenger, which I this is shared between page and sw browser contexts, uses the Page log and always logs. The Service Worker logs more, pictured below, only if activated.

Disabling:

image

Enabling:

image

Checklist

  • I have included screenshots/recordings of the intended results or explained why they are not needed

Related Tickets

#992



This change is Reviewable

@onethirtyfive onethirtyfive force-pushed the sync-page-and-sw-logger-enabling branch 5 times, most recently from 8bfbf5d to e25ece3 Compare March 6, 2023 09:47
@onethirtyfive onethirtyfive force-pushed the sync-page-and-sw-logger-enabling branch from e25ece3 to 83d919b Compare March 6, 2023 09:51
@rgomezp
Copy link
Contributor

rgomezp commented Mar 8, 2023

Hey Joshua, thanks for opening this PR!

So unfortunately we're not planning on making any API changes to this SDK. In part this is due to the new SDK currently in beta.

That said, we also wouldn't add this function to the new SDK since the ideal behavior would more simply be that the one existing logging function setLogLevel sets it on both the page and sw contexts.

We really do appreciate your work here, and if you would like to use your changes, you always can! Simply build the SDK and host the SDK files yourself. You can follow the instructions in this README.

Lastly, if you haven't taken a look at the new beta, I highly encourage that you do so that you can start the migration to the new user-based model, which will allow you to make the most of OneSignal's omni-channel offering to target users from multiple channels.

@onethirtyfive
Copy link
Author

onethirtyfive commented Mar 8, 2023 via email

@rgomezp rgomezp closed this Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants