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

Regression test and fix for issue #773 #774

Merged
merged 9 commits into from
Apr 18, 2021
Merged

Conversation

bcheidemann
Copy link
Contributor

Adds a test which currently fails, demonstrating issue #773

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Let's fix this while you're adding this test.

I think the easiest thing would be to mimic what node does here:

diff --git a/src/filesystem/implementation.js b/src/filesystem/implementation.js
index 3027688..67bc8c0 100644
--- a/src/filesystem/implementation.js
+++ b/src/filesystem/implementation.js
@@ -1873,8 +1873,8 @@ function writeFile(context, path, data, options, callback) {
   if(typeof data === 'number') {
     data = '' + data;
   }
-  if(typeof data === 'string' && options.encoding === 'utf8') {
-    data = Buffer.from(data);
+  if(typeof data === 'string') {
+    data = Buffer.from(data, options.encoding || 'utf8');
   }

   open_file(context, path, flags, function(err, fileNode) {

Node does a bit more work to assert the encoding, but I don't know if we care beyond making sure we set the default encoding to 'utf8' if it's missing.

tests/bugs/issue773.js Outdated Show resolved Hide resolved
@bcheidemann
Copy link
Contributor Author

Let's fix this while you're adding this test.

I think the easiest thing would be to mimic what node does here:

diff --git a/src/filesystem/implementation.js b/src/filesystem/implementation.js
index 3027688..67bc8c0 100644
--- a/src/filesystem/implementation.js
+++ b/src/filesystem/implementation.js
@@ -1873,8 +1873,8 @@ function writeFile(context, path, data, options, callback) {
   if(typeof data === 'number') {
     data = '' + data;
   }
-  if(typeof data === 'string' && options.encoding === 'utf8') {
-    data = Buffer.from(data);
+  if(typeof data === 'string') {
+    data = Buffer.from(data, options.encoding || 'utf8');
   }

   open_file(context, path, flags, function(err, fileNode) {

Node does a bit more work to assert the encoding, but I don't know if we care beyond making sure we set the default encoding to 'utf8' if it's missing.

I've implemented the equivalent of what node is doing, should be good to review now =)

@bcheidemann
Copy link
Contributor Author

Closes #773

src/filesystem/implementation.js Outdated Show resolved Hide resolved
@@ -1869,12 +1869,17 @@ function writeFile(context, path, data, options, callback) {
return callback(new Errors.EINVAL('flags is not valid', path));
}

data = data || '';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@humphd I moved this line of code so that data = 0 would be handled the same as other numbers.

@bcheidemann bcheidemann changed the title test: add regression test for issue #773 Regression test and fix for issue #773 Apr 18, 2021
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

Excellent, thank you.

At some point it would be good to do follow-ups in the future that add tests for these new code paths (data being an Object for the .toString() call, using other encodings, etc).

@humphd humphd merged commit 7bd6e5f into filerjs:master Apr 18, 2021
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