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

Ensure compatibility with older versions of Node.js #21863

Merged
merged 6 commits into from
May 1, 2024
Merged
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
@@ -761,7 +761,8 @@ jobs:
other.test_js_optimizer_verbose
other.test_node_unhandled_rejection
other.test_full_js_library*
core2.test_hello_world"
core2.test_hello_world
core2.test_fs_write_rawfs"
# Run a few test with the most recent version of node
# In particular we have some tests that require node flags on older
# versions of node but not with the most recent version.
4 changes: 2 additions & 2 deletions src/library_nodefs.js
Original file line number Diff line number Diff line change
@@ -276,14 +276,14 @@ addToLibrary({
// Node.js < 6 compatibility: node errors on 0 length reads
if (length === 0) return 0;
try {
return fs.readSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), { position: position });
return fs.readSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), 0, length, position);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do newer versions of node also support this old pattern? If so, can we not just use this older pattern everwhere? Is there any downside? It looks like it creates less gabage since you don't need to create a new option object each call.

If newer versions don't support this pattern then I suppose we would want a runtime check rather than a compile time one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newer versions of Node.js ought to support this too. I guarded this for code size reasons, but perhaps that is negligible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit 65a6ff3 prefers the old API instead.

} catch (e) {
throw new FS.ErrnoError(NODEFS.convertNodeCode(e));
}
},
write(stream, buffer, offset, length, position) {
try {
return fs.writeSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), { position: position });
return fs.writeSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), 0, length, position);
} catch (e) {
throw new FS.ErrnoError(NODEFS.convertNodeCode(e));
}
4 changes: 2 additions & 2 deletions src/library_noderawfs.js
Original file line number Diff line number Diff line change
@@ -149,7 +149,7 @@ addToLibrary({
}
var seeking = typeof position != 'undefined';
if (!seeking && stream.seekable) position = stream.position;
var bytesRead = fs.readSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), { position: position });
var bytesRead = fs.readSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), 0, length, position);
// update position marker when non-seeking
if (!seeking) stream.position += bytesRead;
return bytesRead;
@@ -165,7 +165,7 @@ addToLibrary({
}
var seeking = typeof position != 'undefined';
if (!seeking && stream.seekable) position = stream.position;
var bytesWritten = fs.writeSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), { position: position });
var bytesWritten = fs.writeSync(stream.nfd, new Int8Array(buffer.buffer, offset, length), 0, length, position);
// update position marker when non-seeking
if (!seeking) stream.position += bytesWritten;
return bytesWritten;
4 changes: 2 additions & 2 deletions src/library_wasmfs_node.js
Original file line number Diff line number Diff line change
@@ -188,7 +188,7 @@ addToLibrary({
try {
// TODO: Cache open file descriptors to guarantee that opened files will
// still exist when we try to access them.
let nread = fs.readSync(fd, new Int8Array(HEAPU8.buffer, buf_p, len), { position: pos });
let nread = fs.readSync(fd, new Int8Array(HEAPU8.buffer, buf_p, len), 0, len, pos);
{{{ makeSetValue('nread_p', 0, 'nread', 'i32') }}};
} catch (e) {
if (!e.code) throw e;
@@ -202,7 +202,7 @@ addToLibrary({
try {
// TODO: Cache open file descriptors to guarantee that opened files will
// still exist when we try to access them.
let nwritten = fs.writeSync(fd, new Int8Array(HEAPU8.buffer, buf_p, len), { position: pos });
let nwritten = fs.writeSync(fd, new Int8Array(HEAPU8.buffer, buf_p, len), 0, len, pos);
{{{ makeSetValue('nwritten_p', 0, 'nwritten', 'i32') }}};
} catch (e) {
if (!e.code) throw e;
2 changes: 1 addition & 1 deletion test/fs/test_write.cpp
Original file line number Diff line number Diff line change
@@ -24,7 +24,7 @@ int main()
var pos = lengthBytesUTF8(str);

str = '1234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890';
data = new Uint8Array(100);
data = new Uint8Array(101);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do str.length there instead maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stringToUTF8Array(str, data, 0, lengthBytesUTF8(str)+1);
FS.write(stream, data, 0, lengthBytesUTF8(str)+1, pos, /*canOwn=*/false);

1 change: 1 addition & 0 deletions test/test_core.py
Original file line number Diff line number Diff line change
@@ -5822,6 +5822,7 @@ def test_fs_js_api(self):
self.set_setting("FORCE_FILESYSTEM")
self.do_runf('fs/test_fs_js_api.c', 'success')

@also_with_noderawfs
def test_fs_write(self):
if self.get_setting('WASMFS'):
self.set_setting("FORCE_FILESYSTEM")
12 changes: 8 additions & 4 deletions third_party/closure-compiler/node-externs/fs.js
Original file line number Diff line number Diff line change
@@ -373,10 +373,12 @@ fs.write = function(fd, buffer, offset, length, position, callback) {};
/**
* @param {*} fd
* @param {*} buffer
* @param {Object.<string,number>=} options
* @param {number=} offset
* @param {number=} length
* @param {number=} position
* @return {number}
*/
fs.writeSync = function(fd, buffer, options) {};
fs.writeSync = function(fd, buffer, offset, length, position) {};

/**
* @param {*} fd
@@ -391,11 +393,13 @@ fs.read = function(fd, buffer, offset, length, position, callback) {};
/**
* @param {*} fd
* @param {*} buffer
* @param {Object.<string,number>=} options
* @param {number} offset
* @param {number} length
* @param {number} position
* @return {number}
* @nosideeffects
*/
fs.readSync = function(fd, buffer, options) {};
fs.readSync = function(fd, buffer, offset, length, position) {};

/**
* @param {string} filename