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

Add ContextStore #80

Merged
merged 1 commit into from
Jun 15, 2017
Merged

Conversation

shaoshuai0102
Copy link
Contributor

No description provided.

@coveralls
Copy link

coveralls commented Jun 14, 2017

Coverage Status

Coverage decreased (-0.5%) to 99.468% when pulling 65e4f3d on shaoshuai0102:feat-ctx-store-for-4.x into ae21f52 on koajs:4.x.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 99.468% when pulling 65e4f3d on shaoshuai0102:feat-ctx-store-for-4.x into ae21f52 on koajs:4.x.

@coveralls
Copy link

coveralls commented Jun 14, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 89d5c14 on shaoshuai0102:feat-ctx-store-for-4.x into ae21f52 on koajs:4.x.

@shaoshuai0102 shaoshuai0102 changed the title [WIP] Add ContextStore Add ContextStore Jun 14, 2017
@shaoshuai0102
Copy link
Contributor Author

ping @dead-horse

@coveralls
Copy link

coveralls commented Jun 14, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 1d59173 on shaoshuai0102:feat-ctx-store-for-4.x into ae21f52 on koajs:4.x.

@coveralls
Copy link

coveralls commented Jun 14, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling e008b72 on shaoshuai0102:feat-ctx-store-for-4.x into ae21f52 on koajs:4.x.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e008b72 on shaoshuai0102:feat-ctx-store-for-4.x into ae21f52 on koajs:4.x.

index.js Outdated
assert(typeof opts.store.set === 'function', 'store.set must be function');
assert(typeof opts.store.destroy === 'function', 'store.destroy must be function');
if (opts.ContextStore) {
assert(typeof opts.ContextStore === 'function', 'ContextStore must be a function');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is.class

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And check ContextStore.prototype[method]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only check ContextStore.prototype[method] once

@@ -78,10 +78,8 @@ function formatOpts(opts) {
opts.decode = util.decode;
}

if (opts.store) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should check store first when it's specified.

lib/context.js Outdated
if (this.store) {
assert(typeof this.store.get === 'function', 'store.get must be function');
assert(typeof this.store.set === 'function', 'store.set must be function');
assert(typeof this.store.destroy === 'function', 'store.destroy must be function');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to check store type on every request.
Only assert once at the beginning.

@@ -6,635 +6,654 @@ const should = require('should');
const mm = require('mm');
const session = require('..');
const store = require('./store');
const ContextStore = require('./context_store');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep the old test file and add a new one for context store.

@shaoshuai0102
Copy link
Contributor Author

All issues settled

@coveralls
Copy link

coveralls commented Jun 15, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 4703d0b on shaoshuai0102:feat-ctx-store-for-4.x into ae21f52 on koajs:4.x.

@dead-horse
Copy link
Member

should introduce ContextStore in readme

@@ -118,6 +118,8 @@ app.use(convert(session(app)));

Once you passed `options.store`, session is strong dependent on your external store, you can't access session if your external store is down. **Use external session stores only if necessary, avoid use session as a cache, keep session lean and stored by cookie!**

If your session store requires data or utilities from context, `opts.ContextStore` is alse supported. `ContextStore` must be a class which claims three instance methods demonstrated above. `new ContextStore(ctx)` will be executed on every request.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@coveralls
Copy link

coveralls commented Jun 15, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d86f898 on shaoshuai0102:feat-ctx-store-for-4.x into ae21f52 on koajs:4.x.

@dead-horse
Copy link
Member

LGTM

@dead-horse
Copy link
Member

@shaoshuai0102 please pick this feature to master branch

@dead-horse dead-horse merged commit dd2fa4a into koajs:4.x Jun 15, 2017
@shaoshuai0102 shaoshuai0102 deleted the feat-ctx-store-for-4.x branch June 15, 2017 06:17
@shaoshuai0102
Copy link
Contributor Author

publish a version pls.

I will pick it to master

@shaoshuai0102 shaoshuai0102 mentioned this pull request Jun 15, 2017
@shaoshuai0102
Copy link
Contributor Author

4.2.0

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.

5 participants