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.

Sign up for GitHub

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

Jump to bottom

Bump dependencies (uuid, debug, dev-deps) #217

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Bump dependencies (uuid, debug, dev-deps) #217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5791451
Bump dependencies (uuid, debug, dev-deps)
chrisveness Nov 2, 2021
8801948
Fix 'done()' call in contextstore.test.js
chrisveness Mar 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions .eslintrc
View file
Open in desktop

This file was deleted.

5 changes: 3 additions & 2 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
node_modules
coverage
node_modules/
coverage/
.nyc_output/
*.log
11 changes: 5 additions & 6 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
sudo: false
language: node_js
node_js:
- '7'
- '8'
- '10'
- '12'
script: 'npm run test-travis'
after_script: 'npm install coveralls@2 && cat ./coverage/lcov.info | coveralls'
- "node"
- "lts/*"
after_success:
- c8 -r text-lcov npm test | coveralls

10 changes: 5 additions & 5 deletions example.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ app.keys = ['some secret hurr'];

app.use(session(app));

app.use(function* (next){
if ('/favicon.ico' == this.path) return;
var n = this.session.views || 0;
this.session.views = ++n;
this.body = n + ' views';
app.use(function(ctx, next){
if ('/favicon.ico' == ctx.path) return;
var n = ctx.session.views || 0;
ctx.session.views = ++n;
ctx.body = n + ' views';
});

app.listen(3000);
Expand Down
6 changes: 2 additions & 4 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const debug = require('debug')('koa-session');
const ContextSession = require('./lib/context');
const util = require('./lib/util');
const assert = require('assert');
const uuid = require('uuid/v4');
const { v4: uuid } = require('uuid');
const is = require('is-type-of');

const CONTEXT_SESSION = Symbol('context#contextSession');
Expand Down Expand Up @@ -39,8 +39,6 @@ module.exports = function(opts, app) {
if (sess.store) await sess.initFromExternal();
try {
await next();
} catch (err) {
throw err;
} finally {
if (opts.autoCommit) {
await sess.commit();
Expand Down Expand Up @@ -122,7 +120,7 @@ function formatOpts(opts) {
*/

function extendContext(context, opts) {
if (context.hasOwnProperty(CONTEXT_SESSION)) {
if (context.hasOwnProperty(CONTEXT_SESSION)) { // eslint-disable-line no-prototype-builtins
return;
}
Object.defineProperties(context, {
Expand Down
40 changes: 25 additions & 15 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,32 +15,42 @@
],
"devDependencies": {
"benchmark": "^2.1.4",
"eslint": "3",
"eslint-config-egg": "3",
"istanbul": "0",
"koa": "2",
"mm": "^2.1.0",
"mocha": "^5.2.0",
"c8": "^7.0.0",
"coveralls": "^3.0.0",
"eslint": "^8.0.0",
"koa": "^2.0.0",
"mm": "^3.0.0",
"mocha": "^9.0.0",
"mz-modules": "^2.0.0",
"pedding": "^1.1.0",
"uid-safe": "^2.1.3",
"should": "8",
"supertest": "^3.3.0"
"should": "^13.0.0",
"supertest": "^6.0.0"
},
"license": "MIT",
"dependencies": {
"crc": "^3.4.4",
"debug": "^3.1.0",
"debug": "^4.0.0",
"is-type-of": "^1.0.0",
"uuid": "^3.3.2"
"uuid": "^8.0.0"
},
"engines": {
"node": ">=7.6"
"node": ">=10.0.0"
Copy link

Choose a reason for hiding this comment

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

This makes this a breaking change, which'd mean a new major version - could be worth trying to split this PR up into a couple of smaller ones so non-breaking changes can be landed?

Copy link
Author

Choose a reason for hiding this comment

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

From memory, I think it was dev-deps that prompted the engines.node bump, though I didn't make particular note of it.

I've never been quite sure about whether the engines.node should relate to app + dependencies, or app + dependencies + devDependencies.

I think the only substantive changes (ignoring dev-deps) are uuid & debug; [email protected] specifies engines.node as >=6.0, uuid doesn't mention engines.node.

I think I bumped engines.node to 10+ since eslint required that, but whether engines.node should depend on a devDependency, I'm not sure!

To further complicate the issue, [email protected] specifies engines.node as >=12: but having just run a test, the koa-session example.js runs perfectly well on [email protected].

If the koa-session maintainers feel it worthwhile, the deps could be bumped separately from the dev-deps, but perhaps engines.node should not depend on devDependencies, in which I was mistaken in bumping it up to 10.0.0 and it can validly stay on 7.6.0?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok to upgrade node to 10.0.0 since node<10 is already out of maintenance for a long time.

},
"eslintConfig": {
"env": {
"es6": true,
"node": true,
"mocha": true
},
"extends": "eslint:recommended",
"parserOptions": {
"ecmaVersion": 2017
}
},
"scripts": {
"test": "npm run lint && NODE_ENV=test mocha --exit --require should --reporter spec test/*.test.js",
"test-cov": "NODE_ENV=test node ./node_modules/.bin/istanbul cover ./node_modules/.bin/_mocha -- --exit --require should test/*.test.js",
"test-travis": "npm run lint && NODE_ENV=test node ./node_modules/.bin/istanbul cover ./node_modules/.bin/_mocha --report lcovonly -- --exit --require should test/*.test.js",
"lint": "eslint lib test index.js"
"test": "NODE_ENV=test mocha --exit test/*.test.js",
"cover": "c8 npm test",
"lint": "eslint lib/ test/ index.js"
}
}
9 changes: 3 additions & 6 deletions test/contextstore.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -394,12 +394,12 @@ describe('Koa Session External Context Store', () => {

request(server)
Copy link
Member

Choose a reason for hiding this comment

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

if we remove done, we should return request to ensure this promise resolved.

Copy link
Author

Choose a reason for hiding this comment

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

Is it correct like this? (I'm not familiar with this mode of usage of mocha).

.post('/')
.expect(200)
.end((err, res) => {
if (err) return done(err);
const cookie = res.headers['set-cookie'];
should.not.exist(cookie);
})
.expect(200, done);
done();
});
});
it('should set headers if manuallyCommit() is called', done => {
const app = App({ autoCommit: false });
Expand All @@ -418,9 +418,6 @@ describe('Koa Session External Context Store', () => {
request(server)
.post('/')
.expect('Set-Cookie', /koa\.sess/)
.end(err => {
if (err) return done(err);
})
.expect(200, done);
});
});
Expand Down
2 changes: 1 addition & 1 deletion test/cookie.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -850,7 +850,7 @@ describe('Koa Session Cookie', () => {
before(() => {
app = App({ rolling: true });

app.use(function* () {
app.use(function* () { // eslint-disable-line require-yield
console.log(this.path);
if (this.path === '/set') this.session = { foo: 'bar' };
this.body = this.session;
Expand Down