Bug 1540765 - PersistentCache should do JSON parsing off of the main thread #4879
Conversation
lib/PersistentCache.jsm
Outdated
| @@ -59,9 +59,8 @@ this.PersistentCache = class PersistentCache { | |||
| const filepath = OS.Path.join(OS.Constants.Path.localProfileDir, this._filename); | |||
| const fileExists = await OS.File.exists(filepath); | |||
There was a problem hiding this comment.
Out of curiosity, is it still necessary to do the stat here? Can we do the fetch, and handle the failure status in the event that the file does not exist?
| const json = gTextDecoder.decode(binaryData); | ||
| data = JSON.parse(json); | ||
| const file = await fetch(`file://${filepath}`); | ||
| data = await file.json(); |
There was a problem hiding this comment.
I think these two calls can be joined together
await (await fetch(...)).json();
| @@ -385,7 +385,7 @@ describe("DiscoveryStreamFeed", () => { | |||
|
|
|||
| await feed.loadSpocs(feed.store.dispatch); | |||
|
|
|||
| assert.notCalled(global.fetch); | |||
| assert.calledOnce(global.fetch); // Persistent cache attempts a fetch | |||
There was a problem hiding this comment.
This is kinda weird and might make test debugging confusing since the test description mentions should not fetch.
Can you instead stub out PersistentCache so that call doesn’t happen? It should prevent having to make all the other changes as well.
| await cache.get("foo"); | ||
| assert.notCalled(fakeOS.File.read); | ||
| }); | ||
| it("should catch and report errors", async () => { |
There was a problem hiding this comment.
I think it would be useful to keep the Cu.reportError Call if fetch or json parsing fails.
There was a problem hiding this comment.
the problem was fetch() throws if the cache file doesn't exist yet. So, it would be spammy to report that an an error since it is expected on a cold cache. I'll break up the fetch from the json and only report json parsing errors.
…