-
Notifications
You must be signed in to change notification settings - Fork 58
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
Use high-level library load API #217
base: main
Are you sure you want to change the base?
Changes from 6 commits
d765df3
d877e54
1dd3250
e6fa1c6
61c787a
ca3313c
06121a9
f751f2b
c9b67b6
2497f24
f24f6d2
009a09f
e6e1b38
16e53be
f3b8f1e
11cfb01
678e3ba
5b0e460
11a708e
0260cbd
bc1091c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,25 +25,39 @@ public sealed class NodejsPlatform : IDisposable | |
/// <summary> | ||
/// Initializes the Node.js platform. | ||
/// </summary> | ||
/// <param name="libnodePath">Path to the `libnode` shared library, including extension.</param> | ||
/// <param name="libnode"> | ||
/// Name of the `libnode` shared library. | ||
/// Has to be a full file path when using .NET Framework. | ||
/// </param> | ||
/// <param name="args">Optional application arguments.</param> | ||
/// <param name="execArgs">Optional platform options.</param> | ||
/// <exception cref="InvalidOperationException">A Node.js platform instance has already been | ||
/// loaded in the current process.</exception> | ||
public NodejsPlatform( | ||
string libnodePath, | ||
string libnode, | ||
string[]? args = null, | ||
string[]? execArgs = null) | ||
{ | ||
if (string.IsNullOrEmpty(libnodePath)) throw new ArgumentNullException(nameof(libnodePath)); | ||
|
||
if (Current != null) | ||
{ | ||
throw new InvalidOperationException( | ||
"Only one Node.js platform instance per process is allowed."); | ||
} | ||
|
||
nint libnodeHandle = NativeLibrary.Load(libnodePath); | ||
#if NET7_0_OR_GREATER | ||
var entryAssembly = System.Reflection.Assembly.GetEntryAssembly(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you consider using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even using AssemblyDirectory, you still need to pass an assembly. I'd rather use the system default but I can change the assembly to get the current assembly instead of the entry. |
||
|
||
nint libnodeHandle = | ||
entryAssembly != null | ||
? NativeLibrary.Load(libnode, entryAssembly, null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a little concerned about the lack of functional parity on .NET Framework 4.x. Would it be feasible to add code in this repo's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've implemented a partially-featured replica of the original method, but I can't test it because I don't have access to a Windows machine. |
||
: NativeLibrary.Load(libnode); | ||
#else | ||
if (string.IsNullOrEmpty(libnode)) | ||
throw new ArgumentNullException(nameof(libnode)); | ||
|
||
nint libnodeHandle = NativeLibrary.Load(libnode); | ||
#endif | ||
|
||
Runtime = new NodejsRuntime(libnodeHandle); | ||
|
||
Runtime.CreatePlatform(args, execArgs, (error) => Console.WriteLine(error), out _platform) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be
!NETFRAMEWORK
? The 3-parameter overload ofNativeLibrary.Load()
is present on .NET 6.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code was removed in the newest revision.