Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force expiration only if memo still exists #16

Merged

Conversation

allantokuda-zipnosis
Copy link

This should prevent errors of this type:

TypeError: Cannot set property 'forceExpiration' of undefined

This can happen because this code is executed in a promise error callback, which is not in the same event loop as where the memo is created on line 163. Between the time the memo was created and the time of line 217's execution, line 257 delete memos[key] could have already executed.

This helps ensure stability of applications that pass in an expireOnFailure function.

@chrisronline
Copy link
Owner

Seems legit! Do you mind adding a relevant unit test to cover this change?

@allantokuda-zipnosis
Copy link
Author

I would be happy to! I'm having trouble running the tests. Seems angular is not defined in the test context. All I have done to this point is run npm install and gulp test. Do you know if I need to run some other setup commands?

$ gulp test
[10:02:12] Using gulpfile ~/src/angular-promise-cache/gulpfile.js
[10:02:12] Starting 'test'...
04 05 2017 10:02:12.232:WARN [watcher]: Pattern "/Users/atokuda/src/angular-promise-cache/bower_components/angular/angular.min.js" does not match any file.
04 05 2017 10:02:12.234:WARN [watcher]: Pattern "/Users/atokuda/src/angular-promise-cache/bower_components/angular-mocks/angular-mocks.js" does not match any file.
PhantomJS 1.9.8 (Mac OS X 0.0.0) ERROR
  TypeError: 'undefined' is not an object (evaluating 'angular.module')
  at /Users/atokuda/src/angular-promise-cache/angular-promise-cache.js:35


[10:02:14] 'test' errored after 1.9 s
[10:02:14] Error: 1
    at formatError (/usr/local/lib/node_modules/gulp/bin/gulp.js:169:10)
    at Gulp.<anonymous> (/usr/local/lib/node_modules/gulp/bin/gulp.js:195:15)
    at emitOne (events.js:90:13)
    at Gulp.emit (events.js:182:7)
    at Gulp.Orchestrator._emitTaskDone (/Users/atokuda/src/angular-promise-cache/node_modules/orchestrator/index.js:264:8)
    at /Users/atokuda/src/angular-promise-cache/node_modules/orchestrator/index.js:275:23
    at finish (/Users/atokuda/src/angular-promise-cache/node_modules/orchestrator/lib/runTask.js:21:8)
    at cb (/Users/atokuda/src/angular-promise-cache/node_modules/orchestrator/lib/runTask.js:29:3)
    at removeAllListeners (/Users/atokuda/src/angular-promise-cache/node_modules/karma/lib/server.js:336:7)
    at Server.<anonymous> (/Users/atokuda/src/angular-promise-cache/node_modules/karma/lib/server.js:347:9)
    at Server.g (events.js:273:16)
    at emitNone (events.js:85:20)
    at Server.emit (events.js:179:7)
    at emitCloseNT (net.js:1527:8)
    at _combinedTickCallback (node.js:374:13)
    at process._tickCallback (node.js:401:11)

@chrisronline
Copy link
Owner

Try bower install before running the tests.

@chrisronline
Copy link
Owner

If you're running the tests on OSX Sierra, you'll probably run into this issue as well. If you do and wouldn't mind, bump the version of karma-phantomjs-launcher to 1.0.2. Thanks!

@allantokuda-zipnosis allantokuda-zipnosis force-pushed the force_expiration_only_if_needed branch from a6f8bdc to 3534387 Compare October 20, 2017 20:33
@allantokuda-zipnosis
Copy link
Author

This is updated with a unit test! 🎉

There is now a PhantomJS launcher 1.0.4 - would you like the version bumped again?

@chrisronline
Copy link
Owner

@allantokuda-zipnosis Yes please!

@allantokuda-zipnosis
Copy link
Author

Updated. Is this good to go?

@chrisronline chrisronline merged commit 5a30774 into chrisronline:master Oct 26, 2017
@chrisronline
Copy link
Owner

Released in 0.1.2!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants