[CLOSED] New FileSystem API

[core-ai-bot]

Issue by gruehle
Thursday Oct 31, 2013 at 22:36 GMT
Originally opened as adobe/brackets#5797

---

This pull request adds a new FileSystem API that replaces NativeFileSystem, FileIndexManager and direct calls to brackets.fs.* functions.

This pull request changes the API, but does not add any significant new features or performance gains. These will be done in subsequent pull requests.

This is a big change. It will break some extensions. However, the breakages are minimized through deprecation "shims" for many of the commonly used APIs that have been removed.

See this Brackets-Dev forum thread for an introduction to the changes.

See the FileSystem and FileSystem API Migration wiki pages for more details.

Sign-offs:

• [x]@peterflynn
• [x]@iwehrman
• [x]@gruehle

---

gruehle included the following code: https://github.com/adobe/brackets/pull/5797/commits

[core-ai-bot]

Comment by iwehrman
Friday Nov 01, 2013 at 17:49 GMT

---

Reminder to self: remove the Dropbox impl.

[core-ai-bot]

Comment by iwehrman
Friday Nov 01, 2013 at 21:03 GMT

---

Finished with my first pass.

[core-ai-bot]

Comment by iwehrman
Saturday Nov 02, 2013 at 00:53 GMT

---

I think the only unresolved issues are 1) whether we do, in fact, want resolve to also call back with a stat object; and 2) the incoming ProjectManager.filterFactory method.

[core-ai-bot]

Comment by iwehrman
Saturday Nov 02, 2013 at 00:54 GMT

---

And 3) the proposed tweak to the Directory.getContents callback parameters, adding an additional, possibly null, sparse array of stat errors.

[core-ai-bot]

Comment by gruehle
Sunday Nov 03, 2013 at 15:30 GMT

---

Here are my thoughts:

1. Remove the stat parameter from resolve (see details above)
   2)@peterflynn will be adding ProjectManager.filterFactory. This is a good addition.
2. Here is a specific proposal:

• FileSystemImpl.readdir: the returned stats is an Array, where each value is either an instance of FileSystemStats or a FileSystemError
• Directory.getContents(): the callback has the following signature:

/**
 * Callback function for Directory.getContents
 *
 *`@`param {string} error The error that occurred when reading the contents of the Directory. 
 *               If truthy, no contents are returned.
 *`@`param {Array.<FileSystemEntry>} contents The contents of the Directory. 
 *`@`param {object.<string: string>} statErr If non-null, a map of full path values to the 
 *               error that occurred when calling stat on that path.
 */
function (error, contents, statErr) {
}

If there was a stat error for an entry, that entry is not included in contents, but is added to the statErr. statErr is falsy if there were no stat errors.

Thoughts?

[core-ai-bot]

Comment by iwehrman
Sunday Nov 03, 2013 at 16:15 GMT

---

Alternate proposal for 3)

/**
 * Callback function for Directory.getContents
 *
 *`@`param {?string} error The error that occurred when reading the contents of the Directory. 
 *               If truthy, no contents are returned.
 *`@`param {Array.<FileSystemEntry>=} contents The entire contents of the Directory.
 *`@`param {Array.<string | FileSystemStats=} statsOrErrors An array, consistent with contents,
 *               of stat errors and stat objects. Each entry in contents has either a corresponding
 *               stat error or a corresponding stats object.
 */

Why this might be better:

• If we have the stats object, why not make it available? In future impls, a second stats call could result in another HTTP request.
• It's annoying to iterate over the "own" properties in an object.

[core-ai-bot]

Comment by gruehle
Sunday Nov 03, 2013 at 17:19 GMT

---

@iwehrman how would we create a contents entry if we don't know if it's a File or Directory? I suppose we could create FileSystemEntry objects, but that would violate the rule that every entry is a File or Directory.

[core-ai-bot]

Comment by iwehrman
Sunday Nov 03, 2013 at 18:20 GMT

---

Oh, right. Retracted!

[core-ai-bot]

Comment by iwehrman
Monday Nov 04, 2013 at 01:22 GMT

---

I'm implementing your proposal now and it occurs to me that we still have the option to return the stat objects for successfully stat-ed entries. The final callback parameter could well be of type object.<string: string|FileSystemStats>.

[core-ai-bot]

Comment by iwehrman
Monday Nov 04, 2013 at 01:24 GMT

---

I'm going to push an implementation of your proposed now spec though; we can keep talking tomorrow about whether or not we want to elaborate it further.

[core-ai-bot]

Comment by iwehrman
Monday Nov 04, 2013 at 17:35 GMT

---

I'm also not yet convinced that we should remove the stat parameter from the resolve callback. Sure impls could figure out a way to determine whether an entry is a file or directory without calling stat, but in that case it seems plausible that they would also be able to figure out the size and mtime as well. And let's not forget that we're combining these calls because of the potential-but-significant network overhead. So, given that in the Brackets app shell there isn't a faster way to make this determination, it seems still worth considering.

[core-ai-bot]

Comment by busykai
Monday Nov 04, 2013 at 17:59 GMT

---

@iwehrman -- second that. actual fs impl could do that (e.g. by caching when creating index). there might also be a client-server implementation which could do it in a same remote call (without the need to call twice from the client).

[core-ai-bot]

Comment by gruehle
Monday Nov 04, 2013 at 19:40 GMT

---

After further discussion, we decided to keep the stat parameter in the resolve callback, and add a stats Array to the Directory.getContents() callback.@iwehrman will be making this change.

[core-ai-bot]

Comment by iwehrman
Monday Nov 04, 2013 at 20:10 GMT

---

All of the issues I raised have been resolved, so I'm checking off. Over to you,@peterflynn! 🎾

[core-ai-bot]

Comment by peterflynn
Tuesday Nov 05, 2013 at 02:23 GMT

---

High-level note: there are still 50-60 references to FileEntry and 15+ references to DirectoryEntry in the codebase (presumably all in comments). Should we aim to have those 100% cleaned up this sprint?

[core-ai-bot]

Comment by iwehrman
Tuesday Nov 05, 2013 at 05:19 GMT

---

I'll clean those up tomorrow.

[core-ai-bot]

Comment by iwehrman
Wednesday Nov 06, 2013 at 19:19 GMT

---

@gruehle@peterflynn I've pushed 92ea58b, "Add FileSystemError.TOO_MANY_ENTRIES and a maxEntries option to FileSystemEntry.visit with default 30,000".

[core-ai-bot]

Comment by peterflynn
Thursday Nov 07, 2013 at 05:39 GMT

---

Whew! Finally time to come up for air! Done with first pass.

I haven't looked at any replies to my comments yet. Will start circling back now.

[core-ai-bot]

Comment by gruehle
Thursday Nov 07, 2013 at 21:16 GMT

---

Initial review complete. ☕

[core-ai-bot]

Comment by iwehrman
Thursday Nov 07, 2013 at 22:08 GMT

---

All changes from the review comments pushed.

[core-ai-bot]

Comment by peterflynn
Friday Nov 08, 2013 at 00:43 GMT

---

Sorry for adding a bunch of comments saying I was fixing stuff that's already been fixed... dunno how I got so far out of sync from the tip of the branch. One of my git fetches must have filed without me noticing the error message, or something.

[core-ai-bot]

Comment by peterflynn
Friday Nov 08, 2013 at 01:39 GMT

---

Holy crap, it's actually ready to land. Here goes nothing! 🙈   🙏

[core-ai-bot]

Comment by MiguelCastillo
Friday Nov 08, 2013 at 01:44 GMT

---

Oh man... Let's see how this one breaks my extensions 🎲

[core-ai-bot]

Comment by peterflynn
Friday Nov 08, 2013 at 01:47 GMT

---

Boom! The eagle has landed!

[core-ai-bot]

Comment by iwehrman
Friday Nov 08, 2013 at 01:51 GMT

---

[core-ai-bot]

Comment by peterflynn
Friday Nov 08, 2013 at 01:54 GMT

---

High five, Ian! 🙌

[core-ai-bot]

Comment by njx
Friday Nov 08, 2013 at 01:57 GMT

---

[core-ai-bot]

Comment by larz0
Friday Nov 08, 2013 at 02:40 GMT

---

[core-ai-bot]

Comment by lkcampbell
Friday Nov 08, 2013 at 05:01 GMT

---