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

add appendFile/appendFileSync support #65

Closed
wants to merge 1 commit into from
Closed

add appendFile/appendFileSync support #65

wants to merge 1 commit into from

Conversation

manidlou
Copy link
Contributor

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.

@manidlou manidlou mentioned this pull request Nov 12, 2016
@jprichardson
Copy link
Owner

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

I invite you to read: #23 (comment) and #37 (comment) and I'd love to know your thoughts.

@manidlou
Copy link
Contributor Author

manidlou commented Nov 13, 2016

@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
Copy link
Owner

jprichardson commented Nov 13, 2016

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
Copy link
Contributor Author

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
Copy link
Owner

So, what do you think then?

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

@manidlou
Copy link
Contributor Author

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:

  • appending a javascript object to an existing JSON file
  • appending another JSON file to an existing one.

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

@jprichardson
Copy link
Owner

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
Copy link

farskid commented Apr 21, 2017

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

@RyanZim
Copy link
Collaborator

RyanZim commented Apr 25, 2017

Closing as per discussion in #52.

@RyanZim RyanZim closed this Apr 25, 2017
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.

4 participants