Skip to content

feat(browser): Add IndexedDb offline transport store#6983

Merged
AbhiPrasad merged 23 commits intogetsentry:developfrom
timfish:feat/browser-offline
Feb 8, 2023
Merged

feat(browser): Add IndexedDb offline transport store#6983
AbhiPrasad merged 23 commits intogetsentry:developfrom
timfish:feat/browser-offline

Conversation

@timfish
Copy link
Copy Markdown
Collaborator

@timfish timfish commented Jan 30, 2023 •
edited
Loading

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

@AbhiPrasad AbhiPrasad mentioned this pull request Jan 31, 2023
Closed
9 tasks
@mydea mydea changed the base branch from master to develop February 2, 2023 09:47
@mydea
Copy link
Copy Markdown
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
@AbhiPrasad AbhiPrasad self-requested a review February 6, 2023 10:33
@timfish timfish mentioned this pull request Feb 6, 2023
Merged
@timfish
Copy link
Copy Markdown
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
Copy Markdown
Contributor

AbhiPrasad commented Feb 6, 2023

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
Copy Markdown
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

Lms24
Lms24 approved these changes Feb 8, 2023
View reviewed changes
Copy link
Copy Markdown
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.

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

@Lms24 Lms24 Feb 8, 2023

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

AbhiPrasad
AbhiPrasad approved these changes Feb 8, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@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

Reviewers

@Lms24 Lms24 Lms24 approved these changes

+1 more reviewer

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Can't get Offline integration to work

4 participants

@timfish @mydea @AbhiPrasad @Lms24