Conversation
| await addCssFileToZip(zip, "autoDark.css"); | ||
| let fileList = await getFileList("../plugin/popup.html"); | ||
| let localeNames = await getLocaleFilesNames(); | ||
| ["js/ContentScript.js"].concat(localeNames).concat(fileList.filter(n => !n.includes("/experimental/"))); |
There was a problem hiding this comment.
I think this needs to be
fileList = ["js/ContentScript.js"].concat(localeNames).concat(fileList.filter(n => !n.includes("/experimental/")));So that next line processes the correct list of files
|
|
||
| getChapterUrls(dom, chapterUrlsUI) { | ||
| return this.getChapterUrlsFromMultipleTocPages(dom, | ||
| async getChapterUrls(dom, chapterUrlsUI) { |
There was a problem hiding this comment.
Pretty sure this needs to be
return (await this.getChapterUrlsFromMultipleTocPages(dom,
ComrademaoParser.extractPartialChapterList,
ComrademaoParser.getUrlsOfTocPages,
chapterUrlsUI
)).reverse();| let nextUrl = QinxiaoshuoParser.urlOfNextPageOfChapter(xhr.responseXML, fetchedUrls); | ||
| return QinxiaoshuoParser.fetchPagesOfChapter(finalDom, fetchedUrls, nextUrl); | ||
| }); | ||
| let xhr = await HttpClient.wrapFetch(url); |
There was a problem hiding this comment.
This is using recursion due to use of then. Can be re-written as simple iteration.
| }.bind(this)); | ||
| async responseToJson(response) { | ||
| let data = await response.text(); | ||
| this.json = JSON.parse(data); |
| text: data.toString() | ||
| }); | ||
| var readAllFiles = async function(fileList, loadedFiles) { | ||
| return await fileList.forEach(async function(fileName) { |
There was a problem hiding this comment.
reduce should be used here not a forEach. Return of forEach will be undefined. (if this is an array)
See this Doc on Array.prototype.forEach. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/forEach#return_value
What could be optimized is flattening the functions. Use an await a Promise.All for each of these items,
And each promise item in an array.
After promise.all you can return the array and it will be for all fulfilled promises. But it would throw a error still if anything failed in promise.all
There was a problem hiding this comment.
Regardless.
Would be good to have unit tests of this functionality to prove it's not thread blocking at the await syntax similar to original implementation and that the return is as expected
| }; | ||
|
|
||
|
|
||
| (async () => { |
There was a problem hiding this comment.
Make this a function like init() that can be called. Call it afterwards but wrap it in a process.env variable so init is not called on import/require
This would make the code much more testable by not forcing execution on evaluation.
| }); | ||
| }, Promise.resolve()); | ||
| var addFilesToZip = async function(zip, fileList) { | ||
| return await fileList.forEach(async function(fileName) { |
There was a problem hiding this comment.
Add tests for inputs/outputs please!
3 participants
Change the rest of the
.then()s to async-await.Ran the tests on the first commit, where there are no changes to the test files, and got all the tests to pass.
It's late, and I haven't given the code a once-over for style mistakes, so leaving this as a draft for now.