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

Node.JS workers compatibility #27

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

Conversation

AmeerArsala
Copy link

Here's the updated PR! This one fully has the changes you want!!

@lovell
Copy link
Owner

lovell commented Aug 26, 2024

Hi, thanks for the PR, did you see #25 and the need to use some form of safe-require-with-fallback approach?

@AmeerArsala
Copy link
Author

There! I did it. I tried opening a new PR but it took me here. added safeRequire with fallbacks.

@lovell
Copy link
Owner

lovell commented Aug 28, 2024

Thanks for the updates, as I mentioned in #25 I think the safeRequire function should return null when all options are exhausted (rather than throw), and consumers need to guard against this. Are you able to update the logic to match this behaviour?

Please can you also remove the formatting changes to ensure semistandard will pass, as well as keeping the diff cleaner.

@AmeerArsala
Copy link
Author

Done! My bad, I didn't realize my editor automatically made the formatting changes. It returns null now

@AmeerArsala
Copy link
Author

Any update on this?

@lovell
Copy link
Owner

lovell commented Sep 4, 2024

Given that safeRequire() can return null, consumers need to guard against this, e.g. childProcess might be null. Are you able to update the logic to match this behaviour?

@AmeerArsala
Copy link
Author

Alright, updated! Sorry for the long wait, I just saw this

@AmeerArsala
Copy link
Author

Ok so from my inspection I saw that the for loop I made was the issue. I changed it from being a foreach loop to just a regular for loop in case some versions of Node don't play nice with it in this context.

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