Switch internal directory representation to use maps #1169

winstliu · 2017-08-23T03:25:03Z

Requirements

Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
All new code requires tests to ensure against regressions

Description of the Change

This PR switches the internal representations of directory entries to use Maps instead of Objects. While they are very similar, there is one big disadvantage to using Objects - Objects have internal and inherited properties/keys. Since setting object keys sets the key directly, this means that certain directory names can conflict and override the existing Object properties. A notable example of this is __proto__. Naming a directory __proto__ causes Tree View to override the base Object prototype, which leads to Very Bad Things(tm). Maps do not have this limitation.

Alternate Designs

Switching to Map was the first thing I considered, and it worked. Alternatives would be to refuse to load folders that contained __proto__ entries, to prevent __proto__ entries from being added to the Directory representation, or to use a different data structure. I chose Maps due to the reasons outlined above and because MDN states that Maps should be used if possible when there is the potential for unknown keys.

Benefits

The name of a directory should no longer render Atom unusable.

Possible Drawbacks

This changes the output of onDidRemoveEntries from an Object to a Map. I'm unaware if that's being used anywhere outside of Tree View (see below). In addition, the serialized expansion state has also changed from an Object to a Map, meaning that extra conversion code will need to be added for a smooth transition.

Applicable Issues

Fixes #1079

/cc @Alhadis to make sure that this doesn't affect file-icons (in my testing it didn't)
/cc @Daniel15

TODO:

Specs
Serialization conversion
Make sure this doesn't break any packages

Alhadis · 2017-08-23T03:38:44Z

The same error probably occurs with files that're named __proto__, too. Did you check?

EDIT: Confirmed.

winstliu · 2017-08-23T13:14:06Z

Really? I'm pretty sure I tested out a __proto__ file and nothing broke.

thomasjo · 2017-08-23T13:18:43Z

lib/directory-view.coffee

        view.element.remove()
        subscription.dispose()
-        break
+        # TODO: When this file is converted to JS, convert this forEach loop
+        # to a for...of loop and add a break here for performance


Another option is to avoid the "problem" by using Array.prototype.some instead;

removedEntries.some (removedEntry, removedName) -> return true unless entry is removedEntry view.element.remove() subscription.dispose()

This code will behave essentially exactly like the old version — it will iterate until the desired item is encountered, then short-circuit the iteration. You could of course also achieve this by using Array.prototype.every, but the version above more closely mimics the old version I think (at least in terms of lines of code, and overall legibility).

Nevermind... just realized you're working with Map and not Array — d'oh!

Yep, it's annoying because Coffeescript's for...of is JS's for...in, and of course its for...in is just a fancy for loop and not for...of. So there's no way to loop Map entries. Oh well.

Since you already checked that using Maps does not break consumers of this API, I wonder if we could change the parameters to make this code path more efficient. In particular, what about exposing a Set of entries that were removed so that we can retrieve it in O(1) here? Like:

if (removedEntries.has(entry)) { // cleanup }

An alternative might be to rely on the name and avoid changing the API altogether:

if (removedEntries.has(entry.name)) { // cleanup }

Although I am slightly concerned about the order of events when a rename occurs if we decide to go choose the second option.

What do you think? 💭

could you expand on your concerns about the event order with the second option?

Yeah, sorry if that comment was a little obscure. My primary concern is that in the same tick of the loop the following could happen:

An entry with name x is added.

A previous entry with name x is deleted.

I am not sure this would be possible in the current model, but if we receive change events asynchronously I worry that the add and the delete event could get grouped together and cause the wrong entry (or both, in the example above) to be deleted.

If that's a circumstance that we are sure we will never encounter, then I think we could go ahead and implement 2). Otherwise, relying on reference equality seems to be safer.

Alhadis · 2017-08-23T14:02:35Z

Well, I created the __proto__ file from the tree-view's context-menu, and this happened as soon as I entered the filename:

After reloading the window, I encountered a different error:

winstliu · 2017-08-23T14:43:01Z

@Alhadis are you trying with my PR? The Directory class handles files to some extent as well, which is why I didn't have to change the File implementation.

Alhadis · 2017-08-23T14:56:57Z

Oh! Whoops, my bad. Yeah, your branch handles files as well. Never mind. 👼

winstliu · 2017-08-23T17:21:19Z

Just reminding myself which part of the serialization I need to work on:

Uncaught (in promise) TypeError: this.expansionState.entries.get is not a function
    at Directory.module.exports.Directory.getEntries (file:///C:/Users/user/Documents/GitHub/tree-view/lib/directory.coffee:196:52)
    at Directory.module.exports.Directory.reload (file:///C:/Users/user/Documents/GitHub/tree-view/lib/directory.coffee:231:5)
    at Directory.module.exports.Directory.expand (file:///C:/Users/user/Documents/GitHub/tree-view/lib/directory.coffee:269:6)
    at DirectoryView.module.exports.DirectoryView.expand (file:///C:/Users/user/Documents/GitHub/tree-view/lib/directory-view.coffee:133:18)
    at new DirectoryView (file:///C:/Users/user/Documents/GitHub/tree-view/lib/directory-view.coffee:62:6)
    at TreeView.<anonymous> (file:///C:/Users/user/Documents/GitHub/tree-view/lib/tree-view.coffee:296:14)
    at TreeView.module.exports.TreeView.updateRoots (file:///C:/Users/user/Documents/GitHub/tree-view/lib/tree-view.coffee:277:14)
    at new TreeView (file:///C:/Users/user/Documents/GitHub/tree-view/lib/tree-view.coffee:57:6)
    at TreeViewPackage.getTreeViewInstance (C:\Users\user\Documents\GitHub\tree-view\lib\tree-view-package.js:57:23)
    at Object.deserialize (C:\Users\user\AppData\Local\Atom x64\app-dev\resources\app\src\package.js:562:58)
    at DeserializerManager.deserialize (C:\Users\user\AppData\Local\Atom x64\app-dev\resources\app\src\deserializer-manager.js:95:35)
    at C:\Users\user\AppData\Local\Atom x64\app-dev\resources\app\src\pane.js:48:36
    at Array.map (native)
    at Function.module.exports.Pane.deserialize (C:\Users\user\AppData\Local\Atom x64\app-dev\resources\app\src\pane.js:47:27)
    at DeserializerManager.deserialize (C:\Users\user\AppData\Local\Atom x64\app-dev\resources\app\src\deserializer-manager.js:95:35)
    at PaneContainer.deserialize (C:\Users\user\AppData\Local\Atom x64\app-dev\resources\app\src\pane-container.js:58:44)
    at Dock.deserialize (C:\Users\user\AppData\Local\Atom x64\app-dev\resources\app\src\dock.js:378:30)
    at Workspace.deserialize (C:\Users\user\AppData\Local\Atom x64\app-dev\resources\app\src\workspace.js:416:40)
    at C:\Users\user\AppData\Local\Atom x64\app-dev\resources\app\src\atom-environment.js:1263:35

Alhadis · 2017-08-24T02:17:09Z

lib/directory.coffee

@@ -26,6 +26,16 @@ class Directory
    @isRoot ?= false
    @expansionState ?= {}
    @expansionState.isExpanded ?= false
+
+    # TODO: This can be removed after a sufficient amount
+    # of time has passed since @expansionState.entries


Waiting until after #1146 should be enough, too. 😜

winstliu · 2017-08-28T22:22:44Z

grep -r "onDidRemoveEntries" . found three packages with that text:

https://atom.io/packages/tree-view-no-drag
- Fork that automatically updates based on a script, shouldn't be a problem
https://atom.io/packages/tree-view-remote
- Fork with 0 downloads, last commit Nov. 24, 2014
https://atom.io/packages/greyatom-ide-tree
- Fork maintained by Greyatom IDE

So it looks like only Tree View forks are using onDidRemoveEntries and therefore it should be safe to proceed with this implementation.

Alhadis · 2017-09-01T13:42:10Z

@50Wliu Just an FYI, there were indeed regressions with file-icons; specifically, directory icons were missing, and icons weren't responding to async changes in file/icon mappings. Not to worry though, I've got it all under control.

as-cii

I left one minor comment, but otherwise this looks good to me.

Nice work, @50Wliu! ⚡️

as-cii · 2017-09-06T12:26:05Z

lib/directory-view.coffee

        view.element.remove()
        subscription.dispose()
-        break
+        # TODO: When this file is converted to JS, convert this forEach loop
+        # to a for...of loop and add a break here for performance


Since you already checked that using Maps does not break consumers of this API, I wonder if we could change the parameters to make this code path more efficient. In particular, what about exposing a Set of entries that were removed so that we can retrieve it in O(1) here? Like:

if (removedEntries.has(entry)) { // cleanup }

An alternative might be to rely on the name and avoid changing the API altogether:

if (removedEntries.has(entry.name)) { // cleanup }

Although I am slightly concerned about the order of events when a rename occurs if we decide to go choose the second option.

What do you think? 💭

as-cii · 2017-09-06T12:47:09Z

One more question, related to @Alhadis' comment:

Just an FYI, there were indeed regressions with file-icons; specifically, directory icons were missing, and icons weren't responding to async changes in file/icon mappings. Not to worry though, I've got it all under control.

Are we worried at all about breaking other packages by shipping this pull-request?

Alhadis · 2017-09-06T12:56:06Z

Are we worried at all about breaking other packages by shipping this pull-request?

@as-cii I don't imagine there'd be a risk of runtime breakage. They were POJOs in the first place, which means any package using them would probably only be looping through their properties with a for..in loop.

However, this might lead to subtle regressions in packages which expect to find filenames in the form of enumerated key/value pairs (as file-icons had been).

winstliu · 2017-09-06T22:19:40Z

@as-cii could you expand on your concerns about the event order with the second option?

winstliu · 2017-09-08T12:11:36Z

@as-cii I have implemented the first option, just to be safe.

Switch internal directory representation to use maps

23516fe

winstliu added the work-in-progress label Aug 23, 2017

thomasjo reviewed Aug 23, 2017

View reviewed changes

Serialization conversion

f2712ed

Alhadis reviewed Aug 24, 2017

View reviewed changes

Wliu added 3 commits August 23, 2017 23:22

Let's do some proper recursive conversion

6c2da58

Test conversion logic

c8fc461

Add spec for __proto__ entries

55af47b

winstliu force-pushed the wl-use-maps branch from 71cdef5 to 55af47b Compare August 24, 2017 14:45

winstliu mentioned this pull request Aug 24, 2017

Flaky test: Git status decorations -> when a file is selected in a directory and is deleted, it updates the style of the directory #1172

Open

winstliu added needs-review and removed work-in-progress labels Aug 28, 2017

Alhadis added a commit to file-icons/atom that referenced this pull request Sep 1, 2017

Patch tree-view consumer to prepare for atom/tree-view#1169

a9833cf

as-cii approved these changes Sep 6, 2017

View reviewed changes

🐎

f77b84f

winstliu merged commit d8b04dc into master Sep 14, 2017

winstliu deleted the wl-use-maps branch September 14, 2017 19:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch internal directory representation to use maps #1169

Switch internal directory representation to use maps #1169

winstliu commented Aug 23, 2017 •

edited

Loading

Alhadis commented Aug 23, 2017 •

edited

Loading

winstliu commented Aug 23, 2017 •

edited

Loading

thomasjo Aug 23, 2017

thomasjo Aug 23, 2017

winstliu Aug 24, 2017

as-cii Sep 6, 2017

as-cii Sep 7, 2017 •

edited

Loading

Alhadis commented Aug 23, 2017

winstliu commented Aug 23, 2017 •

edited

Loading

Alhadis commented Aug 23, 2017

winstliu commented Aug 23, 2017

Alhadis Aug 24, 2017

winstliu commented Aug 28, 2017

Alhadis commented Sep 1, 2017 •

edited

Loading

as-cii left a comment

as-cii Sep 6, 2017

as-cii commented Sep 6, 2017

Alhadis commented Sep 6, 2017

winstliu commented Sep 6, 2017

winstliu commented Sep 8, 2017

Switch internal directory representation to use maps #1169

Switch internal directory representation to use maps #1169

Conversation

winstliu commented Aug 23, 2017 • edited Loading

Requirements

Description of the Change

Alternate Designs

Benefits

Possible Drawbacks

Applicable Issues

Alhadis commented Aug 23, 2017 • edited Loading

winstliu commented Aug 23, 2017 • edited Loading

thomasjo Aug 23, 2017

Choose a reason for hiding this comment

thomasjo Aug 23, 2017

Choose a reason for hiding this comment

winstliu Aug 24, 2017

Choose a reason for hiding this comment

as-cii Sep 6, 2017

Choose a reason for hiding this comment

as-cii Sep 7, 2017 • edited Loading

Choose a reason for hiding this comment

Alhadis commented Aug 23, 2017

winstliu commented Aug 23, 2017 • edited Loading

Alhadis commented Aug 23, 2017

winstliu commented Aug 23, 2017

Alhadis Aug 24, 2017

Choose a reason for hiding this comment

winstliu commented Aug 28, 2017

Alhadis commented Sep 1, 2017 • edited Loading

as-cii left a comment

Choose a reason for hiding this comment

as-cii Sep 6, 2017

Choose a reason for hiding this comment

as-cii commented Sep 6, 2017

Alhadis commented Sep 6, 2017

winstliu commented Sep 6, 2017

winstliu commented Sep 8, 2017

winstliu commented Aug 23, 2017 •

edited

Loading

Alhadis commented Aug 23, 2017 •

edited

Loading

winstliu commented Aug 23, 2017 •

edited

Loading

as-cii Sep 7, 2017 •

edited

Loading

winstliu commented Aug 23, 2017 •

edited

Loading

Alhadis commented Sep 1, 2017 •

edited

Loading