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

nw.js support #2053

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

nw.js support #2053

wants to merge 3 commits into from

Conversation

MikalDev
Copy link

This is to allow for node files to also work with Windows nw.js. not just electron and node.
Using the patch from this issue: #1480.
The patch checks if a node library function is available in the executable, if not, it loads the external library version of node via node.dll (which is already loaded.)

(added verification)

Checks if a node library function is available in the executable, if not, it loads the external library version of node via node.dll.
@MikalDev
Copy link
Author

MikalDev commented Apr 18, 2024

The failing workflow (macOS arm64) does not seem related to the change. Anyone else have thoughts on this?

@MikalDev
Copy link
Author

Looks like I need to do: cargo fmt, doing that now.

@Kreijstal
Copy link

Kreijstal commented Apr 21, 2024

this conflicts with this PR #2026, since we use the exact same patch fix, I have not added any modification to the MSVC version, but I use a very similar fix for mingw support. I can also do this change if @Brooooooklyn decides too, remember, that under mingw version we use libnode.dll since the linker prepends lib to most dlls, just like in all gnu projects. The fix would not work on GNU since node.dll is not loaded, so this is why it tries to load it, but if it fails, it then chooses to load it.

@MikalDev
Copy link
Author

MikalDev commented Apr 22, 2024

If same functionality for nw.js support on windows are implemented in PR #2026 , I'm ok with that one being accepted and will close this one after that.

@Kreijstal
Copy link

If same functionality for nw.js support on windows are implemented in PR #2026 , I'm ok with that one being accepted and will close this one after that.

not really, I made sure to single out MSVC, (since I can't test on MSVC), can you provide a test case for nw.js

@MikalDev
Copy link
Author

Ah, I see, I'll take a look at your tests and take a look at doing a PR to your fork to allow it to support MSVC and windows nw.js testing. Or if your PR gets accepted before that, I'll just back to doing a PR vs main.

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.

None yet

3 participants