Making bgQueue compatible with higher level library by prateekbh · Pull Request #170 · GoogleChrome/workbox

prateekbh · 2017-01-23T18:16:22Z

This makes sw-background-sync-queue compatible with higher level library.

# 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 · 2017-01-23T18:16:40Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

jpmedley · 2017-01-23T18:33:46Z

 	}
+
+	/**
+	 * This function is a call wrapper over `pushIntoQueue` used by higher


Change 'This function is a call wrapper over' to 'Wraps the'.

In reference materials, something are not complete sentences.

jpmedley · 2017-01-23T18:35:10Z

+	 * level framework. If you are writting the fetch handler for background
+	 * sync manually, please ignore this.
+	 *
+	 * @param {Object} input


Need a definition. You can probably copy it from pushIntoQueue.

jpmedley · 2017-01-23T18:35:59Z

+	 * @param {Request} input.request The request which is to be queued
+	 *
+	 * @return {Promise} Promise which resolves when the request is pushed in
+	 * the queue


Please add periods to the end of the parameter definitions.

prateekbh · 2017-01-24T10:26:18Z

@jpmedley did the requested changes, please review

gauntface · 2017-01-24T15:01:42Z

One comment from me - otherwise LGTM.

I'm slightly scared of the size of these commit logs. In future could you either do a fresh checkout or ensure you are updating against a branch off of master.

jeffposnick · 2017-01-24T15:08:26Z

-	}
+const route = new goog.routing.ExpressRoute({
+  path: '/*',
+  origin: 'https://jsonplaceholder.typicode.com',


That's not going to work as expected—origin isn't a supported parameter.

As of #158, you can't use ExpressRoute for cross-origin routes. You should use RegExpRoute instead:

const route = new goog.routing.RegExpRoute({ regExp: new RegExp('^https://jsonplaceholder.typicode.com'), handler: new goog.runtimeCaching.NetworkOnly({requestWrapper}), });

@jeffposnick is it something that will change in near future?
cuz i right now it worked...
just curious

As of #158, you can pass in /*, but it won't trigger a match for cross-origin requests. If you saw your route match a cross-origin request, it might have been because you didn't have that updated code in effect.

origin has never been supported in ExpressRoute. It was just ignored.

jeffposnick · 2017-01-24T15:09:03Z

+ *
+ * const route = new goog.routing.ExpressRoute({
+ * 	path: '/*',
+ * 	origin: 'https://jsonplaceholder.typicode.com',


See earlier comment—use RegExpRoute instead.

jpmedley · 2017-01-24T16:37:27Z

@prateekbh @jeffposnick @gauntface @addyosmani

There are parameters that lack definitions. Since they were'nt highlighted in red or green, I didn't request changes. We need to start filling these in.

prateekbh · 2017-01-24T16:40:58Z

@gauntface I am highly curious why is this even happening?
1.) None of the commits are on my profile
2.) Every time I open a PR, it shows all commits.
3.) I regularly merge with this repo's master

jeffposnick · 2017-01-24T16:49:34Z

After you pull in the changes from the upstream, canonical GitHub repo to your local instance of git on your personal machine, are you pushing those changes out to your personal forked GitHub repo?

I think GitHub is showing a diff of your forked repo vs. the canonical repo, so that implies your forked repo doesn't have the commits (even though your local git working copy has them).

prateekbh · 2017-01-27T01:36:41Z

@jeffposnick I've changed the demo sw, please review

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 14 commits January 9, 2017 22:47

fixing comment

b013fb4

changing documentation

0406801

making sense with jsDocs

28c5364

making sense with jsDocs

a29d4b4

completing documentation

1dc08a4

proper demo

078653f

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

integration with higher level framework

c1558c0

updating readme

2cfe608

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

ef9e573

jpmedley suggested changes Jan 23, 2017

View reviewed changes

resolving comments

71b6fab

jeffposnick suggested changes Jan 24, 2017

View reviewed changes

jpmedley approved these changes Jan 24, 2017

View reviewed changes

fixing demo

db9ccf2

jeffposnick approved these changes Jan 27, 2017

View reviewed changes

prateekbh merged commit fb12c27 into GoogleChrome:master Jan 27, 2017

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

Conversation

prateekbh commented Jan 23, 2017

Uh oh!

googlebot commented Jan 23, 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!

prateekbh commented Jan 24, 2017

Uh oh!

gauntface commented Jan 24, 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!

jpmedley commented Jan 24, 2017

Uh oh!

prateekbh commented Jan 24, 2017

Uh oh!

jeffposnick commented Jan 24, 2017

Uh oh!

prateekbh commented Jan 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants