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.

Sign up for GitHub

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

Jump to bottom

NODEFS and NODERAWFS broken since v3.1.45 with node < v18.3.0 #21118

Closed
mzoliker opened this issue Jan 20, 2024 · 5 comments · Fixed by #21863
Closed

NODEFS and NODERAWFS broken since v3.1.45 with node < v18.3.0 #21118

mzoliker opened this issue Jan 20, 2024 · 5 comments · Fixed by #21863

Comments

@mzoliker
Copy link

Hi,

In #20077 the functions fs.readSync and fs.writeSync have been updated with signatures that did not exist in node v10.19.0 (for instance fs.writeSync with options was only added in node v18.3.0), which breaks running with any earlier version of node.

The four occurrences should really be rewritten as follows for them to work as expected as from node v10.19.0:

return fs.readSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), 0, length, position);
return fs.writeSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), 0, length, position);
var bytesRead = fs.readSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), 0, length, position);
var bytesWritten = fs.writeSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), 0, length, position);

Thanks a lot for the fix!

Kind regards,
Maurice
@sbc100
Copy link
Collaborator

sbc100 commented Jan 20, 2024

Good to know that someone is still targeting a version of node that old.

We need to find a way to make those calls work with both wasm64 and old versions of node I guess.

Can I ask why you are targetting such an old version of node? Are you planning on one day dropping support for older versions like that? I'm wondering when its reasonable for us to do that same.

@mzoliker
Copy link
Author

In fact I am using node v18.1.0 (with which my code cannot run as from emscripten v3.1.45), but I need to keep compatibility with node v12.16.0 for historical reasons (run on iOS). That’s why I build with -sMIN_NODE_VERSION=101900 (by the way, not sure which other versions I can safely use other than 101900).

@sbc100
Copy link
Collaborator

sbc100 commented Jan 20, 2024

Can you explain what "run on iOS" means? How do you use node on iOS in your use case?

For sure we need to support node version older than v18.3.0 so we need to fix this issue regardless.

kleisauke added a commit to kleisauke/emscripten that referenced this issue Apr 30, 2024
@kleisauke
Ensure compatibility with older versions of Node.js
8f6e33a 
The previous `fs.writeSync` API was not available in Node.js
17.x, see:
https://nodejs.org/api/fs.html#fswritesyncfd-buffer-options

Also, do the same for `fs.readSync` to ensure compat with
Node.js < v13.13.0.

Resolves emscripten-core#21118.
@kleisauke kleisauke mentioned this issue Apr 30, 2024
Merged
@kleisauke
Copy link
Collaborator

I just opened PR #21863 for this.

sbc100 pushed a commit that referenced this issue May 1, 2024
@kleisauke
Ensure compatibility with older versions of Node.js (#21863)
16fd432 
The previous `fs.writeSync` API was not available in Node.js
17.x, see:
https://nodejs.org/api/fs.html#fswritesyncfd-buffer-options

Also, do the same for `fs.readSync` to ensure compat with
Node.js < v13.13.0.

Resolves #21118.
@mzoliker
Copy link
Author

mzoliker commented May 9, 2024

Hi! Thanks a lot for the fix. When released, I will try to remove the post-processing sed to confirm it now works fine without it :-).
Kind regards,
Maurice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Ensure compatibility with older versions of Node.js kleisauke/emscripten
3 participants
@sbc100 @kleisauke @mzoliker