Re: [w3c/ServiceWorker] Remove incumbent/fetching record from Cache behavior (#1190)

jakearchibald commented on this pull request.



>      </section>
 
     <section algorithm="cache-addAll">
       <h4 id="cache-addAll">{{Cache/addAll(requests)}}</h4>
 
       <dfn method for="Cache"><code>addAll(|requests|)</code></dfn> method *must* run these steps:
 
-        1. Let |responsePromiseArray| be an empty array.
-        1. Let |requestArray| be an empty array.
+        1. Let |r| be null.

Implementations may optimise by reusing variables, but I don't think it's something we should do in the spec as it makes it harder to read. The way it's currently scoped, I expected the second loop to use the current value of |r|. or |r| to be used after both loops.

I think variables should be as shallowly-scoped as possible, so if they're only used in a loop, then they can be scoped to that loop.

In this case, it means |r| will only ever be "let", never "set", so readers can tell it's a constant.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/w3c/ServiceWorker/pull/1190#discussion_r144038003