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

Remove broken check #1157

Merged
merged 4 commits into from
Apr 25, 2023
Merged

Remove broken check #1157

merged 4 commits into from
Apr 25, 2023

Conversation

mrlika
Copy link
Contributor

@mrlika mrlika commented Apr 18, 2023

@pubkey

This PR contains:

  • A BUGFIX

Describe the problem you have without this PR

No window object in a service worker context causes the service worker to fail to detect native support.

The check, for some reason, was not removed in this PR: #1037

Todos

  • Tests
  • Changelog

The check, for some reason, was not removed in this PR: #1037
@pubkey
Copy link
Owner

pubkey commented Apr 24, 2023

This is intentional. The nodejs worker only works in worker threads.

@pubkey pubkey closed this Apr 24, 2023
@pubkey pubkey reopened this Apr 25, 2023
@pubkey
Copy link
Owner

pubkey commented Apr 25, 2023

@mrlika sorry I first missunderstood the problem.
Can you check why the CI is failing?

@mrlika
Copy link
Contributor Author

mrlika commented Apr 25, 2023

The test needs to be corrected. In Node v18 BroadcastChannel is defined in the global scope as a native implementation. If the native implementation is used, clearNodeFolder should do nothing as in the browser context.

@mrlika
Copy link
Contributor Author

mrlika commented Apr 25, 2023

https://github.com/pubkey/rxdb/issues/852 test also fails because it thinks that in Node v18 environment for channel1 node method is used by default, and it fails on fs.unlinkSync(channel1._state.socketEE.path);

@mrlika
Copy link
Contributor Author

mrlika commented Apr 25, 2023

@pubkey, I fixed failing tests to force the use of the node method in the isNode environment because after my change default (autodetected) method for Node.js v18 is native (not node as it was before)

Please confirm that in Node.js v18 default should be native and not node.

@pubkey
Copy link
Owner

pubkey commented Apr 25, 2023

In Node.js, there is the BroadcastChannel API but that only works in workers, not across node.js processes.
In node.js it should not use the native method by default, but the "node" method.

Pinning the method for some tests is not a solution, it should autodetect the correct method.

@mrlika
Copy link
Contributor Author

mrlika commented Apr 25, 2023

Ok. Let me think about how to handle it.

@mrlika
Copy link
Contributor Author

mrlika commented Apr 25, 2023

Ok. Fixed.

  • typeof window !== 'undefined' is for regular browser environment
  • typeof self !== 'undefined' is for Service and Web Workers environments
  • then check typeof BroadcastChannel === 'function' to fallback to indexedDB implementation in Safari <= 15.3 and other old browsers, which are missing BrowserChannel API

This condition filters Node.js environment with no window and self objects. Please ensure that the condition works for Node.js workers as well.

@pubkey pubkey merged commit 7487f87 into pubkey:master Apr 25, 2023
@pubkey
Copy link
Owner

pubkey commented Apr 25, 2023

Merged and released @mrlika . Thank you, please test the new release.

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