session

session copied to clipboard

There are still a few(actually a lot of the same) errors on the standard that I'm not understanding how to fix (90% are the test/session.js).

here's just a few:

• [x] session/index.js:87:7: 'options' is already defined
• [x] session/index.js:162:59: Unexpected use of comma operator.
• [x] session/session/store.js:30:1: The '__proto__' property is deprecated.
• [x] session/test/session.js:17:1: 'describe' is not defined.
• [x] session/test/session.js:18:3: 'it' is not defined.
• [x] session/test/session.js:27:20: Expected consistent spacing
• [x] session/index.js:447: Unexpected mix of '||' and '&&' no-mixed-operators
• [ ] session/test/session.js:2350: Unnecessary escape character: \. no-useless-escape

We probably want to merge as many PRs before doing this to this module, otherwise it is going to make it almost impossible to accept any of the current PRs (especially since this module was not using a format even close to standard before). Perhaps part of the 1 -> 2 version of this module? Thoughts?

As for some of the errors, they should be OK if you want to use the template I linked to in the standard issue (the changes do not really load on my phone well, so not sure).

Oh that's true we should accept the other pull requests before merging this. I'll update it when we start to push towards v2.

Hey @gabeio, if you rebase on top of https://github.com/expressjs/session/commit/16cbfb29860499470d4daa3f2e4b886b62f1fb79 then the __proto__ stuff should be gone now :)

Done 😄 yes the __proto__ error is gone 👍

@dougwilson should I make the changes, just like you made in that commit to this repository also; the package.json & .travis.yml edits that force it to run checks using the eslint?

Yep :)

done 👍 (three of the tests I had to add an if (err) return done(err))

Nice! Doing a rebase should now resolve the second item on the list.

And now item 1 should be resolved as well.

I also just cherry-picked out your commit https://github.com/gabeio/session/commit/a893cb3ec19b4c9101a97ba6be7e8cf7f010d4ad since that is useful outside of just converting to Standard format.

I rebased and squashed them. (without the commit you cherry-picked 👍 )

---

I'll continue to rebase as the master updates 👍 .

feel free to add the [needs rebase] tag back to this pr as needed.

@dougwilson there are some new errors/suggestions in the standardjs lint:

/home/travis/build/expressjs/session/index.js
  284:15  error  'new Buffer()' was deprecated since v6. Use 'Buffer.alloc()' or 'Buffer.from()' (use 'https://www.npmjs.com/package/safe-buffer' for '<4.5.0') instead  node/no-deprecated-api
  447:27  error  Unexpected mix of '||' and '&&'  no-mixed-operators
  447:65  error  Unexpected mix of '||' and '&&'  no-mixed-operators
/home/travis/build/expressjs/session/test/session.js
  2350:31  error  Unnecessary escape character: \.  no-useless-escape
  2350:37  error  Unnecessary escape character: \.  no-useless-escape

I think I have a stash somewhere with the Buffer change, but not the others.

The unnecessary escape character seems possibly incorrect.

If the . character is within a character class [ ] then it doesn't need to be escaped, which is probably what the issue is.