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(browser): Add IndexedDb offline transport store #6983

Merged
merged 23 commits into from Feb 8, 2023

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Jan 30, 2023

Closes #3046

This PR:

  • Adds insert/pop wrappers for IndexedDb
    • I used idb-keyval as reference since it's well used and tested
  • Adds createIndexedDbStore that creates an OfflineStore implementation
  • Adds a transport wrapper (makeIndexedDbOfflineTransport) that supplements transport options with IndexedDbOptions
  • Exports makeBrowserOfflineTransport for use in creating browser offline transport
  • Uses fake-indexeddb to test the code in node.js

How it's used:

import { init, makeBrowserOfflineTransport, makeFetchTransport } from '@sentry/browser';

init({
  dsn: '__DSN__',
  transport: makeBrowserOfflineTransport(makeFetchTransport),
});

Browser support:

  • IE is not supported due to use of IndexedDb getAllKeys
  • IE is likely not supported anyway due to buggy IndexedDb and the requirement of global TextEncoder/TextDecoder

Since IE is not supported we could make the fetch transport the default so it does not need to be passed to makeFetchTransport?

Tests

@mydea mydea changed the base branch from master to develop February 2, 2023 09:47
@mydea
Copy link
Member

mydea commented Feb 2, 2023

Note: I changed the target to develop, which is the new main branch.

@timfish timfish marked this pull request as ready for review February 5, 2023 18:53
@timfish
Copy link
Collaborator Author

timfish commented Feb 6, 2023

Just noticed that this offline wrapper is currently getting included in the default browser bundles and adds around 2-3KB.

Options:

  • Do nothing and deal with the increase
  • Move this feature to another package
  • Move this feature to a named export (like @sentry/browser/offline)

@AbhiPrasad
Copy link
Member

Just noticed that this offline wrapper is currently getting included in the default browser bundles

The CDN bundle? I think we're fine with that sacrifice. For npm they can just tree-shake it away.

@timfish
Copy link
Collaborator Author

timfish commented Feb 8, 2023

Options:

  • Do nothing and deal with the increase
  • Move this feature to another package
  • Move this feature to a named export (like @sentry/browser/offline)

We went with a 4th option. Exclude makeBrowserOfflineTransport from CDN bundles

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM and good that we're excluding it from the bundles.

@@ -187,8 +187,12 @@ export function makeTSPlugin(jsVersion) {
* from the browser and browser+tracing bundles.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: Let's just update the JSDoc here that this factory function can be used to exclude basically any exported member from the bundles

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As part of the offline integration deprecation, we also need to revamp all of our docs for this!

@AbhiPrasad AbhiPrasad merged commit b539b36 into getsentry:develop Feb 8, 2023
@timfish timfish deleted the feat/browser-offline branch February 13, 2023 19:32
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.

Can't get Offline integration to work
4 participants