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

feat: add support for nested objects to fromJSON() #535

Merged
merged 6 commits into from May 19, 2020

Conversation

FlorianLoch
Copy link
Contributor

This adds support for nested object (literals) to fromJSON(). It reads much nicer and makes tests look cleaner (at least that is my personal opinion). One of the features I miss most not being able to use mock-fs.

I added this functionality to fromJSON() itself, one could argue having an additional method like fromNestedJSON() might be better regarding to backward compatibility. I don't think this is a problem at all, but the rather exotic corner case of calling fromJSON() with something like {'dir1': {'dir2'}} just created dir1 so far, but would also create dir2 from now on...

Feedback on this is highly appreciated!

Additionally this fixes a bug (at least I think it's one): so far fromJSON() only took the cwd parameter into account when creating files, not when creating directories.

Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Awesome, this will be cool.

I actually wrote a utility function for this back when I first started using memfs that I meant to pr back, and it looks very similar aside from using the array methods for looping.

I'll include it here if you want to have a look - looking over it I'm pretty sure they're identical in logic 😂

code
import * as path from 'path';

type Contents = string | null;

export interface Directory {
  [key: string]: Directory | Contents;
}

/**
 * Flattens an object representing nested directories, so that it can be used with `memfs`.
 *
 * `memfs` expects a single level JSON object, with directories being represented by slashes.
 * This function generates such an object from a nested object.
 *
 * `root` is the "root" of the dir - by default this is the actual root ('/').
 * You should only have to manually set this if you merge directories, using the spread operator.
 *
 * @example
 *  vol.fromJSON(
 *    flatDirJSON({
 *      dir: {
 *        'file.txt': 'contents'
 *      }
 *    })
 *  );
 *
 * @param {Directory | null} dir
 * @param {string} [root='/']
 *
 * @return {Record<string, string>}
 */
export const flatDirJSON = (dir: Directory | null, root = '/'): Record<string, Contents> => {
  if (dir === null) {
    return { [root]: dir };
  }

  const flattenDir: Record<string, Contents> = {};

  Object.keys(dir).reduce<Array<[string, Contents]>>((files, folderName) => {
    const flattenName = path.posix.join(root, folderName);
    const folderValue = dir[folderName];

    const directory: Array<[string, Contents]> = (
      typeof folderValue !== 'string'
        ? Object.entries(flatDirJSON(folderValue, flattenName))
        : [[flattenName, folderValue]]
    );

    return files.concat(directory);
  }, []).forEach(([name, value]) => flattenDir[name] = value);

  return flattenDir;
};

src/volume.ts Outdated Show resolved Hide resolved
src/volume.ts Outdated Show resolved Hide resolved
Copy link
Owner

@streamich streamich left a comment

Choose a reason for hiding this comment

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

I wonder if it could be a separate function or a separate library that given nested object keys flattens it?

const data = {
  dir1: {
    file: 'foobar',
  },
};

flattenObject(data);
/*
{
  'dir1/file': 'foobar'
}
*/

@streamich
Copy link
Owner

@G-Rath ah, you beat me :D

@G-Rath
Copy link
Collaborator

G-Rath commented May 13, 2020

you beat me

I think we clicked submit on the same second 😂

I wonder if it could be a separate function or a separate library

I don't have a strong preference - I think it's nice to have fromJSON doing the flattening, but in saying that the function might be a better starting point since otherwise you raise the question of what to do with toJSON.

I feel while it's not a big deal, it's still a deal that fromJSON takes a nested structure (regardless of how it works internally), and then toJSON always returns a flattened one.

Maybe the answer is fromNestedJSON?


I did just think of a good question tho: what happens when you pass in an array, since that's an object 😬

@FlorianLoch
Copy link
Contributor Author

For me the best two options are integrating it into fromJSON() directly or adding a utility function. The first way seems preferential to me, as I think it's quite intuitive to call fromJSON() with a nested object. I actually tried it like that initially before I realized that a flat object is required.
But if you prefer fromNestedJSON() is also fine with me.

Regarding passing an array: well, it will not really work as expected - but that's the case right now too :D
I mean it's JavaScript - passing wrong arguments usually does not fail (gracefully).

@FlorianLoch
Copy link
Contributor Author

I did some general cleanup to the fromJSON() method - I think its much easier to read now. If I got a little too excited I am sorry ;)

@streamich
Copy link
Owner

I'm a bit concerned about overloading fromJSON function, maybe we can instead create another method? Like fromNestedJSON:

fromNestedJSON(dir) {
  return this.fromJSON(flatDirJSON(dir));
}

@FlorianLoch
Copy link
Contributor Author

FlorianLoch commented May 14, 2020

Ok, I think its more intuitive that way but it's of course your decision ;)
If adding it as extra method, should there be an additional static method on Volume too?
And would you want to expose the (internal) helper flattenDirJSON()?

@streamich
Copy link
Owner

@FlorianLoch We can do only the static method, without the instance method. Let's not expose flattenDirJSON for now.

Copy link
Owner

@streamich streamich left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good, just one questions and a nit see below.

Comment on lines +910 to +913
fromNestedJSON(json: NestedDirectoryJSON, cwd?: string) {
this.fromJSON(flattenJSON(json), cwd);
}

Copy link
Owner

@streamich streamich May 15, 2020

Choose a reason for hiding this comment

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

Since we have a similar static method, do we need this one on the instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just thought it would be more consistent this way two have both methods as static and as member ones.

src/volume.ts Show resolved Hide resolved
src/volume.ts Outdated Show resolved Hide resolved
Copy link
Owner

@streamich streamich left a comment

Choose a reason for hiding this comment

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

Thank you!

@streamich streamich merged commit 2c38b01 into streamich:master May 19, 2020
@streamich
Copy link
Owner

🎉 This PR is included in version 3.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants