jprichardson / node-jsonfile

Easily read/write JSON files.
MIT License
1.2k stars 321 forks source link

add appendFile/appendFileSync support #65

Closed manidlou closed 7 years ago

manidlou commented 7 years ago

Hi @jprichardson,

this PR is regarding the issue #52 (append file support).

I've added two functions, appendFile and appendFileSync, and their tests. I've also added the node v7 to the .travis.yml and appveyor.yml.

I appreciate it if you look at them. I'd like to know what you think about it. Thanks.

jprichardson commented 7 years ago

Hi @mawni - I appreciate your support and help here.

I invite you to read: https://github.com/jprichardson/node-jsonfile/pull/23#issuecomment-115241476 and https://github.com/jprichardson/node-jsonfile/pull/37#issuecomment-154750420 and I'd love to know your thoughts.

manidlou commented 7 years ago

@jprichardson thanks for considering this. I really appreciate it.

So, I agree with you that writeFile and appendFile should be consistent. In other words, since writeFile deals with one JSON file, then appendFile should also follow the same pattern.

Accordingly, I just tested the appendFileSync function of PR #23 and #37, and as you mentioned they both just appending another JSON record to the existing one. I really appreciate the authors' work, but the result was not even a valid JSON. Please correct me if I am doing something wrong there!

However, as you know better than me, appendFile and appendFileSync are in high demand from jsonfile users. Moreover, as I read various discussions regarding this issue, although still not 100% clear what users want, what I understood (and kind of make a little more sense to me) that appending can be similar to the concept of Object.assign(), Array.concat() if there is data with the same key, and adding new more data if there is not, to the existing JSON file.

For instance, in #52, as @sahas- mentioned,

is there a way to do append new objects to a file?

and also @ajsingh007 mentioned,

I have the same question. Its working well for me but it overwrites the existing json rather than adding to it

and also what @dragonbanshee mentioned in #23,

I can append to a json file the user's name, their id, the time, etc.

The moreDataToAppend can be in the form of a javascript object or read from another JSON file. In both cases, the resulting existing JSON file will have its own stuff plus the newly added data, still as a one JSON file and I don't think this would be inconsistent with writeFile.

For example,

const jf = require('jsonfile')
var existingFile = './somefile.json'
/*
somefile = {
  garply: 'plugh',
  baz: {
    qux: ['corge']
  }
}
*/
var moreDataToAppend = {
  foo: 'bar',
  baz: {
    id: 09,
    qux: ['waldo', 'thud']
  }
}
// (array concatenation can also be provided as an option)
jf.appendFile(existingFile, moreDataToAppend, options, callback)
/*
  the resulting existing file:
    {
      garply: 'plugh',
      foo: 'bar',
      baz: {
        id: 09,
        qux: ['corge', 'waldo', 'thud']
      }
    }
*/

So, please let me know what you think. Thanks.

jprichardson commented 7 years ago

I think this would be really confusing, as I believe most users are thinking that appendFile would append a new JSON object to a new line.

Maybe as a workaround, we just modify the README and explain both cases? Also, couldn't an appendFile be created by just setting the flag to 'a'? a la:

const jsonFile = require('jsonfile')

jsonFile.writeFile(file, someObj, { flag: 'a' }, callback)

i.e. maybe we just need to modify the docs? Thoughts?

edit:

This is how appendFile is implemented already... https://github.com/nodejs/node/blob/195989d3a3057c54da49b16276151a47a54e500d/lib/fs.js#L1244

manidlou commented 7 years ago

Alright, thanks for clarity. Now it makes more sense.

So, @jprichardson you are right about {flag: 'a'}. I just tested with it and it appended to the new line as a new JSON object to an existing one.

Now, as you pointed out in the previous discussions, in all cases appending deals with multiple JSON objects that is a different paradigm from writeFile. For instance, one of the main changes is the file extension when you append JSON object(s) to an existing one.

For example, as mentioned in ndjson,

When saved to a file, the file extension SHOULD be .ndjson.

Therefore, I confirm that it is somehow confusing and it is indeed inconsistent with writeFile.

In addition, about the README, I think generally it is a good idea to provide more clarity regarding this.

So, what do you think then?

jprichardson commented 7 years ago

So, what do you think then?

I like a README update documenting the various ways to append.

manidlou commented 7 years ago

Alright, I am trying to update the README regarding appending. I was thinking providing two different examples for these cases that uses {flag: 'a'} for writeFile:

Furthermore, in those examples, what should be the extension of the resulting JSON file after appending?

jprichardson commented 7 years ago

Using something like Object.assign, it's fine to retain .json extension. If the JSON is per line a la {flags: 'a'}, it should be .ndjson.

farskid commented 7 years ago

Where is this PR going? Is it gonna be merged soon? Seems like a useful case to me

RyanZim commented 7 years ago

Closing as per discussion in https://github.com/jprichardson/node-jsonfile/issues/52.