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: move indexedDB to service worker #696

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

aatmanvaidya
Copy link
Collaborator

@aatmanvaidya aatmanvaidya commented Feb 21, 2025

this PR fixes - #694

TODO

  • update parcel build commands
  • make a function for chrom.runtime.sendMessage() in browserUtils
  • test properly on firefox
  • run the init slurs function also at the start of BG

@aatmanvaidya aatmanvaidya linked an issue Feb 21, 2025 that may be closed by this pull request
@aatmanvaidya
Copy link
Collaborator Author

@dennyabrain @maanasb01 broad updates on the fix made in this PR

image

above is how our message passing is looking right now,

  • Once the extension is installed, the background script initialises the IndexedDB schema.
  • anytime any indexedDB related operation has to be made, content script sends a message to the background script, which then performs the necessary CRUD actions and optionally returns data to the content script.
  • The commands in package.json have been refactored to support these changes. I’ve tested everything locally and also built the extension to verify that all features work as expected in production. @maanasb01 if possible could you test it on your end as well?
  • A weird thing happening is also, in the background.js terminal, we are not able to see the indexedDB from the Chrome Dev tools, but the DB is getting initialised and data is getting stored (was able to verify because all features are working and can see it locally also).
  • With these changes, I tested on 10–15 websites, and the data is no longer being replicated locally.

@maanasb01
Copy link
Collaborator

hey @aatmanvaidya, yeah sure! I can also try the changes locally on my end!

} else if (enableSlurReplacement) {
processPage(location.href);
try {
const response = await chrome.runtime.sendMessage({
Copy link
Collaborator Author

@aatmanvaidya aatmanvaidya Feb 21, 2025

Choose a reason for hiding this comment

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

@dennyabrain since the background script doesn’t have access to tabs, we need to use chrome.runtime.sendMessage() instead of chrome.tabs.sendMessage() to send an message from content-script to background.js.

I had a quick question—we’ve created a module to handle cross-browser compatibility, where Firefox requires using browser instead of chrome.

But when I tested this on Firefox chrome.runtime.sendMessage() seems to work fine there. do you think we should still move this logic into our module and use browser for Firefox, or is it fine as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

can you research this more? I find it odd that an object named chrome is available on firefox. Maybe something else is happening?
regardless now that we have a module for cross browser compatibility, the content script should invoke functions via that module as opposed to directly accessing chrome/browser.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

found the documentation for it - link

Firefox supports both the chrome and browser namespaces
As a porting aid, the Firefox implementation of WebExtensions supports chrome using callbacks and browser using promises. This means that many Chrome extensions work in Firefox without changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok then lets stick to browser only? and use promises coz that is the newer API

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.

Solve indexedDB duplication for each webpage
3 participants