Add support for locking #106
Conversation
|
Just realized I've only covered |
|
@grant Can you please review this PR? |
grant
left a comment
grant
left a comment
There was a problem hiding this comment.
Thanks for sending this to me. I added some comments below.
src/Service.gs
Outdated
| Service_.EXPIRATION_BUFFER_SECONDS_ = 60; | ||
|
|
||
| /** | ||
| * The number of seconds that a token should remain in the cache. |
| }; | ||
|
|
||
| /** | ||
| * Sets the lock to use when checking and refreshing credentials (optional). |
There was a problem hiding this comment.
Instead of:
Sets the lock ... (optional).
say:
Sets an optional lock ...
There was a problem hiding this comment.
This pattern is already used extensively in the library, don't see a lot of value in switching.
| * Using a lock will ensure that only one execution will be able to access the | ||
| * stored credentials at a time. This can prevent race conditions that arise | ||
| * when two executions attempt to refresh an expired token. | ||
| * @param {LockService.Lock} lock The lock to use when accessing credentials. |
There was a problem hiding this comment.
Add:
@see https://developers.google.com/apps-script/reference/lock/
src/Service.gs
Outdated
| }; | ||
|
|
||
| /** | ||
| * Locks access to a block of code, if a lock has been set on this service. |
test/mocks/lock.js
Outdated
| var waitingFibers = []; | ||
|
|
||
| var MockLock = function() { | ||
| this.gotLock = false; |
There was a problem hiding this comment.
Usually these boolean are written has* as in hasLock or hasOwnProperty. gotLock sounds awkward.
See: https://google.github.io/styleguide/jsguide.html#naming-method-names
There was a problem hiding this comment.
hasLock is already defined, and one of the methods it's mocking, hence my search for another name. Replaced with "hasLock_".
test/test.js
Outdated
| expires_in: 100, | ||
| refresh_token: 'bar' | ||
| }; | ||
| var properties = new MockProperties(); |
There was a problem hiding this comment.
Rather than declaring:
var properties = new MockProperties();
properties.setProperty(...)twice, can you allow MockProperties to accept default properties? It would make sense and is small 2 line change.
var MockProperties = function(store) {
this.store = store || {};
this.counter = 0;
};then create MockProperties.
var properties = new MockProperties({'oauth2.test': JSON.stringify(token)});There was a problem hiding this comment.
Done. Switched to using it in most cases, but not all.
test/test.js
Outdated
| }; | ||
|
|
||
| var refreshToken = function() { | ||
| var service = OAuth2.createService('test') |
There was a problem hiding this comment.
This same exact service is being created twice in this file on line 157. Extract this into function.
There was a problem hiding this comment.
Seems more coincidental than anything, I'd rather have it defined locally for clarity.
package.json
Outdated
| "gas-local": "^1.3.0", | ||
| "gulp": "^3.9.0", | ||
| "gulp": "^3.9.1", | ||
| "gulp-bump": "^0.3.1", |
There was a problem hiding this comment.
While you're cleaning up deps, gulp-bump doesn't seem to be used.
There was a problem hiding this comment.
Good catch!
src/Service.gs
Outdated
| throw new Error('Offline access is required.'); | ||
| } | ||
| var headers = { | ||
| 'Accept': this.tokenFormat_ |
There was a problem hiding this comment.
No quotes in keys ('Accept'):
https://google.github.io/styleguide/jsguide.html#features-objects-mixing-keys
There was a problem hiding this comment.
Done, although the rule is about mixing them, not about not having them at all. I think quotes may actually be more appropriate here, since it represents a dict.
| this.resultFunction = () => ''; | ||
| }; | ||
|
|
||
| MockUrlFetchApp.prototype.fetch = function(url, optOptions) { |
There was a problem hiding this comment.
- Please document these methods. I don't know what these methods (giving 0 and '') do.
- The params don't seem to be used.
There was a problem hiding this comment.
They are mocking out the equivalent functions in UrlFetchApp, added a note. I decided to keep the same method signature, even though this implementation currently isn't use those parameters.
|
Thanks for the review, PTAL. |
2 participants
Fixes #105