Add VFS context aware sorting #189
Conversation
src/utils/vfs.js
Outdated
| @@ -182,7 +191,10 @@ export const humanFileSize = (bytes, si = false) => { | |||
| * @param {string} [options.sortDir='asc'] Sort in this direction | |||
| * @return {Object[]} | |||
| */ | |||
| export const transformReaddir = ({path}, files, options = {}) => { | |||
| export const transformReaddir = ({path}, files, capabilityCache, options = {}) => { | |||
There was a problem hiding this comment.
Instead of passing capabilities here, maybe an option could be added here instead ?
Like options.serverSorting, which is just passed from capabilityCache.
This way we don't introduce any state checks to utility functions.
There was a problem hiding this comment.
I have committed some changes, although it seems there are still some cognitive complexity.
There was a problem hiding this comment.
there are still some cognitive complexity
Feel free to make suggestions on this :)
There was a problem hiding this comment.
Sorry, but I did not come up with a better solution.
src/vfs.js
Outdated
| showHiddenFiles: options.showHiddenFiles !== false, | ||
| filter: options.filter | ||
| filter: options.filter, | ||
| serverSorting: capabilityCache[mountPoint(pathToObject(path))] ? capabilityCache[mountPoint(pathToObject(path))].sort : false |
There was a problem hiding this comment.
I'll defer to Anders' judgement on this, but I think this could stand to be broken up with new lines since it is so long.
There was a problem hiding this comment.
I'll defer to Anders' judgement on this, but I think this could stand to be broken up with new lines since it is so long.
How about below code? @ajmeese7 @andersevenrud
const handleDirectoryList = (path, options) => result =>
Promise.resolve(result.map(stat => createFileIter(stat)))
.then(result => {
let capability = capabilityCache[mountPoint(pathToObject(path))];
return transformReaddir(pathToObject(path), result, {
showHiddenFiles: options.showHiddenFiles !== false,
filter: options.filter,
serverSorting: capability.sort ? capability.sort : false
});
});There was a problem hiding this comment.
@mahsashadi you can change:
serverSorting: capability.sort ? capability.sort : falseTo the following:
serverSorting: capability.sort || falseTo reduce the redundancy of it a bit.
Also I recommend changing the variable from a let to a const since there is no mutation of the contents.
mahsashadi
requested review from
andersevenrud and
ajmeese7
and removed request for
andersevenrud and
ajmeese7
mahsashadi
requested review from
andersevenrud
and removed request for
ajmeese7
|
Hi again @andersevenrud |
|
@mahsashadi I'm very sorry, but I got sick last week and was not able to take a look at your stuff. Scheduled another look for this weekend. |
|
Hi again after long time. |
Due to this point, we'd better to improve client-side sorting.
In new code, client will sort only if VFS server adapter does not sort before.