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

Workaround firefox worker init fail by limiting firefox workers to 2 #122

Conversation

pkong-ds
Copy link
Collaborator

@pkong-ds pkong-ds commented Aug 28, 2024

ref MUP-178

@YayunHuang I also experienced firefox always fail, which can be resolved by quitting firefox -> open again
EDIT: by resolved, I mean seeing code changes have effects in localhost

I really don't like what I do in this PR, but guess this workaround

  • enable staging/prod to be used in firefox;
  • although significantly lower the preview speed.

Sidenote - maybe we should add some Recommend to use on Chrome?

@pkong-ds pkong-ds force-pushed the mup-178-firefox-preview-worker-initialization-fail-in-firefox-only branch from d3e784f to ad34ae1 Compare August 28, 2024 08:31
@pkong-ds
Copy link
Collaborator Author

@YayunHuang Sor~ this might conflict with your upcoming changes

Copy link
Contributor

@YayunHuang YayunHuang left a comment

Choose a reason for hiding this comment

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

Agree we should add some recommendation! but not sure where to put it?

Comment on lines 237 to 239
this.maxWorkers = isUserAgentFirefox()
? MAX_FIREFOX_WEB_WORKERS
: navigator.hardwareConcurrency;
Copy link
Contributor

Choose a reason for hiding this comment

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

should also set to MAX_FIREFOX_WEB_WORKERS if navigator.hardwareConcurrency not support
https://caniuse.com/hardwareconcurrency

@pkong-ds
Copy link
Collaborator Author

@YayunHuang Updated, thanks! (CI pending)

Copy link
Contributor

@YayunHuang YayunHuang left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@YayunHuang YayunHuang self-assigned this Aug 28, 2024
@YayunHuang YayunHuang merged commit 49056d9 into oursky:main Aug 28, 2024
1 check passed
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