-
Notifications
You must be signed in to change notification settings - Fork 492
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
fix: import / importing items count #1660
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you @Gozala! Tested in Firefox and Electron, both work as expected.
The "working" spinner is really nice, it solves the awkward "nothing happens for 1-2s" state on slower machines 👍
FYSA we have a basic end-to-end in https://github.com/ipfs-shipyard/ipfs-webui/blob/master/test/e2e/files.test.js which could be extended. But I agree, that should not block this PR, can be added later, unblocking ipfs-desktop release is the priority.
@rafaelramalho19 @jessicaschilling ok with merging this and shipping ipfs-webui v2.11.3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested in Chrome (Mac) - LGTM, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, my suggestion is not mandatory, it's just a small improvement if you agree with it 😄
const directory = groupedEntries.get(`${name}/`) | ||
if (directory) { | ||
directory.entries.push(entry) | ||
directory.size += entry.size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Wouldn't it be best to not mutate the Map item but instead set it again? Maybe something like
if (directory) {
groupedEntries.set(`${name}/`, {
type: 'directory',
size: directory.size + entry.size,
path: name,
entries: [...directory.entries, entry]
})
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish JS had a better immutable data structures, but unfortunately it does not, and using data structures that it has as immutable does come with a overhead. Not mutating map would add quite an overhead, and if we do mutate map but not the items in it we do still mutate so we won't gain much. So when you can't avoid mutations, I lower my bar to referential transparency goal or more simply no side effects. groupByPath
function here encapsulates all the mutations internally so they don't leak outside (same input will produce same output).
I am happy to reconsider given an argument, but as things stand now I feel like we'd be adding overhead but not really gaining anything meaningful.
I'm merging this as-is and starting release dance to unblock ipfs/ipfs-desktop#1661 (comment) |
This fixes #1659, I would like to figure out how to automate tests for that issue, but I also don't want to block release so submitting this now & will try to figure out how to test in the followup.
According to @jessicaschilling we expect that when you drop or select
n
files for importing it would show "Importing n items" and once import is done show "Imported n items", however if you drop / select a file and a directory of n files it should show "Importing 2 items" and later "Imported 2 items" (Unless you had more items imported prior).However code seemed to count number of import tasks for showing "importing n items" and then counting number of total files for showing "Imported n items", so neither was correct. Additionally non-existing
state
field was accessed which lead toNaN
.This change factors out general logic of grouping by directory name used in the view and uses it to count both pending and imported item numbers.