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

Offscreen db #316

Closed
wants to merge 4 commits into from
Closed

Offscreen db #316

wants to merge 4 commits into from

Conversation

jbukl
Copy link

@jbukl jbukl commented Nov 11, 2023

moving db from sw to offscreen

originally I was planning on just reflecting all of the methods/handlers for dictionaryDatabase and keeping translator in SW, but I ended up just moving translator as well and manually writing all of the methods/handlers once I realized several methods would actually need explicit serialization. I don't really like how it currently looks - it feels like service-worker.js is really unwieldy with how many methods are in it. The bunch of message handlers is also irritating.

I also don't like the need to serialize - it might be possible to avoid some of it though through a refactor, but I'm mixed on what to do because it would probably mean more duplicated code because of firefox support. generic serializing could be better rather than in each handler.

haven't tested thoroughly - stuff could be broken.

Copy link

github-actions bot commented Nov 11, 2023

✔️ No visual differences introduced by this PR.

View Playwright Report (note: open the "playwright-report" artifact)

themoeway-bot
themoeway-bot previously approved these changes Nov 12, 2023
Copy link
Collaborator

@praschke praschke left a comment

Choose a reason for hiding this comment

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

this looks good. i tested scanning with dictionary media on chrome and firefox and nothing seems broken.

@djahandarie
Copy link
Collaborator

@jbukl Feel free to mark this PR as ready for review, I think it's good enough to merge assuming that it's working!

@praschke praschke mentioned this pull request Nov 12, 2023
@jbukl
Copy link
Author

jbukl commented Nov 12, 2023

checked the payloads in the offscreen handlers - had a typo in text replacements, and realized that serializing and deserializing RegExp isn't as simple as just new RegExp(regex.toString()) - the addition of flags makes it require additional work for deserialization.

for now I just used an example from SO, but something like https://github.com/yahoo/serialize-javascript could be good if we want to centralize all the serialization (i.e. of ESM Map and Set as well) work.

@jbukl jbukl changed the title WIP Offscreen db Offscreen db Nov 12, 2023
@jbukl jbukl marked this pull request as ready for review November 12, 2023 19:06
@jbukl jbukl requested a review from a team as a code owner November 12, 2023 19:06
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.

5 participants