Adding more tests by prateekbh · Pull Request #92 · GoogleChrome/workbox

prateekbh · 2016-12-19T12:05:15Z

Adding more tests for background sync queue.
Also working on jsDocs

# 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.

gauntface · 2017-01-03T22:10:49Z

    buildPath: 'build/test/constants.js',
    projectDir: __dirname,
  }),
+  buildJSBundle({


Are these all being build so they can used in tests?

yeah, is thr a better way to do it?

Ping to @gauntface for best practices around test builds.

gauntface · 2017-01-03T22:11:48Z


+/**
+ * set the dbName to store the queue and requests
+ * defaults to defaultDBName


Would there ever be a need to set the database back to the default name?

not really, but is used to set the DB name first time

gauntface · 2017-01-03T22:12:19Z

 import {getDbName} from './background-sync-idb-helper';

+/**
+ * Class which maily interacts with outer service worker


This doesn't explain what this class does.

changed, please review

gauntface · 2017-01-03T22:13:33Z

 	pushIntoQueue({request}) {
 		assert.isInstance({request}, Request);
-		this._queue.push({request});
+		return this._queue.push({request});


Is return needed here?

yep, it returns the underlying promise.
if the dev wants to catch the error or wants to wait upon it

Could you update the @return {void} JSDoc to reflect that?

gauntface · 2017-01-03T22:14:37Z

 }

+/**
+ * takes JSON object and return a Request object to be executed by


Couldn't you say "Takes and object" rather than "takes a JSON object".

The input is parsed as JSON or anything.

gauntface · 2017-01-03T22:14:56Z

 * whose maxAge has expired
 *
 * @memberOf Queue
+ * @return {void}


Nit: This isn't needed.

done, earlier lint was throwing an error

gauntface · 2017-01-03T22:17:05Z

 	}

+	/**
+	 * saves the logical queue to DIB


Nit: IDB not DIB?

gauntface · 2017-01-03T22:22:34Z

+			'redirect': 'follow',
+		};
+
+		return queueUtils.getFetchableRequest({idbRequestObject: reqObj})


Whats the difference between get QueueableRequest and getFetchableRequest?

getQueueableRequest => takes Request and makes it into JSON objec

getFetchableRequest => takes JSON and constructs Request Object

gauntface · 2017-01-03T22:24:00Z

Looking through the tests, I can see a few places where the constructor values are checked, but I didn't see any testing of queuing requests and make sure they eventually get made and fail requests get re-attempted - do these tests exist or do they still need writing?

prateekbh · 2017-01-12T04:58:00Z

@jeffposnick @gauntface
I got the documentation working and realized the documentation is a real mess in this PR. Apologies for that.

Please suggest how to go about it.
1.) I can scrap the entire PR and redo the documentation
2.) You can review the tests and code change to accept this, while I open another PR for just documentation

jeffposnick · 2017-01-12T15:02:33Z

I wouldn't worry about the documentation as part of this PR. (None of the packages are in great shape, honestly, and that's one of the things we were going to look into tomorrow.)

I'll take another look at the actual code now!

jeffposnick · 2017-01-12T15:16:38Z

 	pushIntoQueue({request}) {
 		assert.isInstance({request}, Request);
-		this._queue.push({request});
+		return this._queue.push({request});


Could you update the @return {void} JSDoc to reflect that?

jeffposnick · 2017-01-12T15:17:28Z


-let bcmanager;
+/**
+ * broadcasts the message with the guven type and url


"guven" ➡️ "given"

jeffposnick · 2017-01-12T15:19:46Z

+ *
+ * @param {Object} config
+ */
+function broadcastMessage(broadcastManager, {type, url}) {


These parameters look odd. Can you normalize things so that broadcastManager is take in as a named property in a single Object parameter, alongside type and url?

jeffposnick · 2017-01-12T15:22:14Z

+ * @param {String} hash
+ * @return {Object} response
+ */
 async function getResponse(hash) {


Can you wrap hash in an Object parameter, too? I know it's not as useful when there's only one parameter, but it's more confusing to have some methods take Objects and some take position parameters, and it also makes it easier to add in additional parameters later on.

prateekbh · 2017-01-13T08:33:23Z

@jeffposnick fixed my documentation.
Its pretty decent than earlier
https://github.com/prateekbh/sw-helpers/tree/master/packages/sw-background-sync-queue

jeffposnick · 2017-01-17T19:11:54Z

I didn't have any additional comments about the code that need to be addressed in this PR. Given the scope and history of this PR, I'm fine with merging and addressing follow-ups in smaller chunks.

@gauntface, anything you'd like to see resolved before this PR is merged?

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.

gauntface suggested changes Jan 3, 2017

View reviewed changes

prateekbh added 10 commits January 4, 2017 11:45

comments fixing

13e55b9

adding broadcastChannel differently

726c0cb

adding sw-background-queue into build file

ad8516b

adding sw-background-queue into build file

b3f12ea

complete build file

b15464a

fixing bcManager tests

1288605

replay requests tests

25daa9e

more tests

9dcf2b0

completing tests

3d58c12

fixing comment

b013fb4

prateekbh added 4 commits January 12, 2017 11:05

changing documentation

0406801

making sense with jsDocs

28c5364

making sense with jsDocs

a29d4b4

completing documentation

1dc08a4

jeffposnick suggested changes Jan 12, 2017

View reviewed changes

proper demo

078653f

jeffposnick added the testing label Jan 12, 2017

prateekbh added 5 commits January 13, 2017 11:37

resolving comments

5fb2c0c

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

358b060

fixing test

97875fa

changing build file to avoid meory problems

4e8dc2d

lint fixes

b89535f

jeffposnick approved these changes Jan 17, 2017

View reviewed changes

gauntface merged commit 86729ff into GoogleChrome:master Jan 18, 2017

Conversation

prateekbh commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gauntface commented Jan 3, 2017

Uh oh!

prateekbh commented Jan 12, 2017

Uh oh!

jeffposnick commented Jan 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prateekbh commented Jan 13, 2017

Uh oh!

jeffposnick commented Jan 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants