sw-background-sync-queue #72

prateekbh · 2016-11-18T18:41:39Z

R: @jeffposnick @addyosmani @wibblymat @gauntface @ebidel

sw-background-sync-queue library review

# Conflicts: # package.json

* Deploy to GitHub Pages * First attempt at jsdoc integration * Just removing docs for now - creates a lot of noise * Fixing linting issues

* Deploy to GitHub Pages * Removing jsdoc and docs * Fixes unit tests * Adding browser download to gulpfile * Minor console log tweak * Updating selenium assistant to make gulp download work

* First attempt and minified build * Removing old modules

* Changing minification for babili * External sourcemap and tidy up of code * Adding license header file * Removing unused dep

* Pulling out minified JS Bundle * Moving to shared build config. Still needs some work * Moving to modular gulp taks and shared build * Removing travis typo * Removing travis TODO

* WIP * Some more updates to match the latest spec * Latest sw-routing updates * Latest sw-runtime-caching updates * Add .min. to output .js file * Broadcast cache update changes * Use .bind() to ensure this is set inside of callback * Syntax tweaks * Demo updates * CacheWrapper => RequestWrapper * CacheWrapper => RequestWrapper * Lint cleanup

JSDoc for the sw-broadcast-cache-update project.

googlebot · 2016-11-25T04:39:51Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

jeffposnick

Some high-level comments:

We're preferring const over let elsewhere in the project, so it would be good to switch those except for cases where you need let.

There are a ton of things that should be picked up via linting, and I'm not flagging those now.

jeffposnick · 2016-11-29T17:32:26Z

gulp-tasks/test.js

@@ -31,7 +31,7 @@ gulp.task('download-browsers', function() {
  });
 });

-gulp.task('test', ['download-browsers'], () => {
+gulp.task('test', ['download-browsers'],  () => {


Remove extra space.

jeffposnick · 2016-11-29T17:34:17Z

packages/sw-background-sync-queue/README.md

@@ -0,0 +1,2 @@
+# sw-background-sync-queue


FYI, you can run gulp documentation:projects --project=sw-background-sync-queue to generate some meaningful content for this file and then commit that.

i'll keep this for the end, cuz i guess this needs me to write jsDoc for all

jeffposnick · 2016-11-29T17:35:58Z

packages/sw-background-sync-queue/build.js

+ limitations under the License.
+*/
+
+/*


Remove the double license comment.

jeffposnick · 2016-11-29T17:38:42Z

packages/sw-background-sync-queue/demo/index.html

@@ -0,0 +1,46 @@
+<html>


There are various linting things here that would need to be cleaned up—putting the script in an external .js file will make that easier, since you can lint that.

still skipping demo folder, this will be scrapped in the end

jeffposnick · 2016-11-29T17:46:21Z

packages/sw-background-sync-queue/src/index.js

+ * @param {Object=} broadcastChannel
+ * @param {Object=} dbname
+ */
+async function initialize({broadcastChannel, dbName}) {


Consider using the assert library to validate these parameters if they're required.

they are not required params, how do i assert them?
with simple ifs?

jeffposnick · 2016-11-29T18:55:36Z

packages/sw-background-sync-queue/src/lib/request-queue.js

+	 *
+	 * @memberOf Queue
+	 */
+	async getRequestFromQueue(hash) {


Consider using the assert library to validate these parameters if they're required.

jeffposnick · 2016-11-29T18:55:40Z

packages/sw-background-sync-queue/src/lib/request-queue.js

+	 *
+	 * @memberOf Queue
+	 */
+	async getRequestFromQueue(hash) {


Can you switch to using an anonymous object with named properties instead of position parameters?

jeffposnick · 2016-11-29T18:58:17Z

packages/sw-background-sync-queue/src/lib/request-queue.js

+	 * @memberOf Queue
+	 */
+	async getRequestFromQueue(hash) {
+		if(this._queue.indexOf(hash)===-1) {


This would be more readable if refactored as

if(this._queue.includes(hash)) { return this._idbQHelper.get(hash); }

jeffposnick · 2016-11-29T19:01:30Z

packages/sw-background-sync-queue/src/lib/request-queue.js

+	}
+
+	get queue() {
+		return Object.assign([], this._queue);


It's odd seeing Object.assign() used for a [] target.

What's going on here?

returning a copy of the existing queue, making sure that any operations on the returned object should not polute the the private variable.
i guess i should add a comment about the same

jeffposnick · 2016-11-29T19:01:58Z

packages/sw-background-sync-queue/src/lib/response-manager.js

+	let _idbQHelper = getDb();
+	// TODO: change desrialization of headers
+	idbObject.response = {
+		headers: JSON.stringify(response.headers),


See my previous comment about deserialization.

done and removed TODO

jeffposnick · 2016-12-02T20:35:16Z

Thanks for making those changes and addressing my feedback!

Is there anything else that you'd like to finish up prior to doing an initial merge? I'm fine with getting what's currently written into the master branch and then iterating from there.

(There are some style differences in your code that's not being flagged by our current linting setup. I don't think that's something you need to address in this PR, but something that can be addressed in a later PR that tightens our linting rules.)

prateekbh · 2016-12-03T02:10:55Z

@jeffposnick Yay! Thanks for approving.
I guess it'll be best if we merge this and then I send smaller PRs over this :)

jeffposnick and others added 30 commits September 25, 2016 22:25

Super-early exploration

98ae347

Merge remote-tracking branch 'origin/master' into future-of-sw-tooling

52110ff

# Conflicts: # package.json

Move to require() instead of import for the build scripts

87fe548

Adding an assert library

ec0ce6f

Bump up the documentation dependency

95b6e93

Add some configuration/assertions

f00ec72

Add some configuration/assertions

60fa088

Another checkpoint.

872e7e4

More bits.

5e2dd5c

Bug fixes!

069e638

CacheManager -> CacheWrapper

4a9e960

Manager -> Behavior

6410bf6

Pass the (non-false) result of when to the handler as {params}

45d4af0

Manager -> Behavior

3141f33

Add a RegExpRoute class

921de52

Bug fixes + cleanup

1c5d7dc

Docs (#46)

72030af

* Deploy to GitHub Pages * First attempt at jsdoc integration * Just removing docs for now - creates a lot of noise * Fixing linting issues

Gulp clean (#49)

b8721c2

* Deploy to GitHub Pages * Removing jsdoc and docs * Fixes unit tests * Adding browser download to gulpfile * Minor console log tweak * Updating selenium assistant to make gulp download work

Minify (#53)

25a9194

* First attempt and minified build * Removing old modules

First commit

58c1cf8

enabling es6 modules in sw-background-queue

600d384

Changing minification for babili (#55)

25ad7ba

* Changing minification for babili * External sourcemap and tidy up of code * Adding license header file * Removing unused dep

Updating to latest google styleguide (#56)

087ff4b

Files for Request enqueue

ba36c40

Cleaner build (#57)

d15a581

* Pulling out minified JS Bundle * Moving to shared build config. Still needs some work * Moving to modular gulp taks and shared build * Removing travis typo * Removing travis TODO

First working prototype

12ac264

JSDoc for the sw-broadcast-cache-update project.

22ad291

Linting updates

0cd475b

Merge pull request #60 from GoogleChrome/jsdoc

4bb7353

JSDoc for the sw-broadcast-cache-update project.

prateekbh added 3 commits November 23, 2016 20:22

migrating to new build system

1b7391d

Instance model for every request

4b6467e

id based default queues

6aa8bba

prateekbh added 11 commits November 25, 2016 16:48

adding test for queue js

3ae8a67

splitting modules

c030c92

splitting modules

5bea768

Merge branch 'master' of https://github.com/prateekbh/sw-helpers

3884f3d

Merge branch 'master' of https://github.com/prateekbh/sw-helpers

df7be61

queue tests

b7cc7c4

Merge branch 'master' of https://github.com/prateekbh/sw-helpers

08c7d8b

tag based sync events

db70a99

bug fixing cleanup queue

1ab8955

changing callbackName for successfull bgQ request Execution

1768c9d

removing unwanted parameter from push method

665f642

jeffposnick suggested changes Nov 29, 2016

View reviewed changes

prateekbh added 7 commits November 30, 2016 09:45

incorporating comments and more test

9cb137e

lint fixes

d6fe778

bug fixes

1891f91

adding assert to files

79d0ccf

commenting assert for non required params

d86761a

changing lets to consts

2f67488

Decentralising idb helper

dde20ce

jeffposnick approved these changes Dec 2, 2016

View reviewed changes

bug fix

8046eef

jeffposnick merged commit 879e12e into GoogleChrome:master Dec 5, 2016

addyosmani modified the milestone: Service Worker Framework (beta) Feb 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sw-background-sync-queue #72

sw-background-sync-queue #72

prateekbh commented Nov 18, 2016

googlebot commented Nov 25, 2016

jeffposnick left a comment

jeffposnick Nov 29, 2016

prateekbh Nov 30, 2016

jeffposnick Nov 29, 2016

prateekbh Nov 30, 2016

jeffposnick Nov 29, 2016

prateekbh Nov 30, 2016

jeffposnick Nov 29, 2016

prateekbh Nov 30, 2016

jeffposnick Nov 29, 2016

prateekbh Nov 30, 2016 •

edited

Loading

jeffposnick Nov 29, 2016

prateekbh Nov 30, 2016

jeffposnick Nov 29, 2016

prateekbh Nov 30, 2016

jeffposnick Nov 29, 2016

prateekbh Nov 30, 2016

prateekbh Nov 30, 2016

jeffposnick Nov 29, 2016

prateekbh Nov 30, 2016

jeffposnick Nov 29, 2016

prateekbh Nov 30, 2016

jeffposnick commented Dec 2, 2016

prateekbh commented Dec 3, 2016

sw-background-sync-queue #72

sw-background-sync-queue #72

Conversation

prateekbh commented Nov 18, 2016

googlebot commented Nov 25, 2016

jeffposnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekbh Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffposnick commented Dec 2, 2016

prateekbh commented Dec 3, 2016

prateekbh Nov 30, 2016 •

edited

Loading