Support multiple download #1
Conversation
| } | ||
| }); | ||
| }; | ||
|
|
||
| session.on('will-download', listener); | ||
| if (!sessionListenerMap.get(session)) { | ||
| sessionListenerMap.set(session, true); |
There was a problem hiding this comment.
This looks like a session leak where we'll never remove this?
There was a problem hiding this comment.
Don't worry, We just set it one time because of condition if (!sessionListenerMap.get(session))
There was a problem hiding this comment.
I'm not sure what you meant. if we don't have code to remove this. sessionListenerMap would just keep growing infinitely = session leak.
We need to also remove the session when its no longer used?
There was a problem hiding this comment.
I think we just have 1 session / electron windows?. This sessionListenerMap will destroyed when session destroy?
There was a problem hiding this comment.
session is dead doesn't mean it'll be removed from the sessionListenerMap, right? So we should listen to window close event and remove session from sessionListenerMap?
There was a problem hiding this comment.
I updated the code to delete session in sessionListenerMap before windows close event
index.js
Outdated
| const listener = (e, item, webContents) => { | ||
| const url = decodeURIComponent(_.first(item.getURLChain())); |
There was a problem hiding this comment.
The URL created is different than handlerMap.set(decodeURIComponent(url) ?
We should have a utility function to create the consistent key
There was a problem hiding this comment.
we can not use same function because handleMap.set use download url - first time
When start download, the downloader will check the url & follow to new url if has redirect response.
We just have 1 way to get original url is get from item.getUrlChain() - first item
There was a problem hiding this comment.
Not quite sure what you meant, the way hashmap works is that you use the same key, if we cannot use the same key, then there's no point using a hashmap?
There was a problem hiding this comment.
value of item.getUrlChain() (first item) is equal with url that set in L144
There was a problem hiding this comment.
That's not very clear that it'll be equal? Create a function and use it consistently?
There was a problem hiding this comment.
I think it's difference context. So we can not use same way to get it. The first when we set it, We know exactly it's a key. the second time when we use it. The context of code is in download handler. We just have only way to get it is based on item.getChainUrl
There was a problem hiding this comment.
If that's the case, let's break it up into two variables, one for key and one for url to avoid confusion?
index.js
Outdated
| @@ -143,6 +152,7 @@ module.exports.download = (win, url, options) => new Promise((resolve, reject) = | |||
|
|
|||
| handlerMap.set(decodeURIComponent(url), {options, resolve, reject}); | |||
There was a problem hiding this comment.
Do we want to also rename url to key? Do we actually use the url?
the code is based on PR sindresorhus#36