Replaces jshint with eslint #30

jwerre · 2021-10-11T16:38:42Z

Replaced jshint with eslint as discussed in #27

jwerre · 2021-10-11T16:45:10Z

I've add the following two lines to the lint config because I didn't want to mess with the code base too much.

      "no-control-regex": "off",
      "no-prototype-builtins": "off",

Ideally these are removed in the future.

jankapunkt

I have added some detailed comments on things I haven't seen in detail before.
Also I'd like to know how you all think about moving away from var in favour of const and let?

.eslintrc

jankapunkt · 2021-10-11T17:44:58Z

.eslintrc

+        "error", 
+        { 
+          "vars": "all",
+          "args": "none",


I also think we should throw on unused args, at least the jshint did that before

Personally don't think we should throw, it should be a warning.

Personally, I'd like to keep it this way. Though an arg may not be used it makes a function a bit more expressive when you can see what is available.

@jwerre I thought this is thrown when an arg is defined but not used in the function body like so:

function dosomething (foo, bar) { console.log(foo) // bar is defined as arg but never used in the function }

.eslintrc

HappyZombies · 2021-10-11T19:06:01Z

With eslint replacing jshint, it will close this PR #12

jwerre · 2021-10-11T19:09:01Z

This "no-control-regex": "off" is the result of this line:

node-oauth2-server/lib/validator/is.js

Line 11 in b9aae53

    
           UNICODECHARNOCRLF: /^[\u0009|\u0020-\u007E|\u0080-\uD7FF|\uE000-\uFFFD|\u10000-\u10FFFF]+$/,

From what I can tell there are no tests for lib/validator/is.js at all. I'd like to remove the rule but we need to make some test for is.js first.

jankapunkt · 2021-10-12T05:58:50Z

I will add the test and push it to this branch

jankapunkt · 2021-10-12T07:31:24Z

I actually found a faulty usage of the RegExp 😱

the regex groupings use | as separater (logical or) but this should only be used in this way: (\u0009|\u0020) when used in this way [\u0009|\u0020] it actually accepts a | character. I am currently writing the tests in a way that we iterate all 1024 charcodes (generated), and for the UNICODECHARNOCRLF all supported ranges up to 1114111,so we can make sure these regex are working correct.

Fortunately I took a look at. Maybe we should report this to the original project back?

jankapunkt · 2021-10-12T08:44:56Z

I updated the validator and it has now 100% coverage plus we test for all unicode charatcer ranges (valid and invalid ones) so there should be no false-positives or false-negatives sneak in anymore.

I wonder if this could be considered a security vulnerability? I mean it won't allow code injection (am I right?) but at least it would have allowed invalid characters and depending on where it's used could lead to issues.

jankapunkt · 2021-10-12T08:48:16Z

I forgot to mention: the test now takes about ~~6 seconds~~ 11 seconds, which is still not so good but I am not sure how otherwise test the full unicode ranges (including multiple chars) against the RegEx in shorter time...

I also mostly sticked to ES5 code style, although we could easily support ES6 without any issue (since we don't support these super old node versions, right!?) but I think it's better to have a later PR with update to ES6 or even do it when the time for Typescript comes.

HappyZombies · 2021-10-12T11:38:18Z

Maybe we should report this to the original project back?

I mean we can, but as per usual, I guarantee nothing will be done about it.

It would be interesting what issues this Regex might raise if used incorrectly.

lib/validator/is.js

jwerre · 2021-10-12T17:32:41Z

@HappyZombies @jankapunkt I think were good to merge here.

jankapunkt · 2021-10-12T17:36:40Z

From my end it's good I just wanted to @HappyZombies to take a look since I also worked on the code and hence am biased

HappyZombies

@jwerre I left a comment stating that jshint should be npm uninstalled. Not sure if you missed it or I did this review incorrectly lol

package.json

HappyZombies · 2021-10-12T17:51:48Z

Things look good but I want to do a more thorough review later today. Should have this merged by EOD though 👍

HappyZombies · 2021-10-12T23:30:54Z

I am trying to run the linter but am getting the following error.

I am on Windows

Node -> v12.16.3

Error below.

PS node-oauth2-server> npm run pretest

> @node-oauth/oauth2-server@4.0.0 pretest /node-oauth2-server
> eslint lib test index.js


Oops! Something went wrong! :(

ESLint: 8.0.0

Error: Cannot find module '@eslint/eslintrc/universal'
Require stack:
- node-oauth2-server\node_modules\eslint\lib\linter\linter.js
- node-oauth2-server\node_modules\eslint\lib\linter\index.js
- node-oauth2-server\node_modules\eslint\lib\cli-engine\cli-engine.js
- node-oauth2-server\node_modules\eslint\lib\eslint\eslint.js
- node-oauth2-server\node_modules\eslint\lib\eslint\index.js
- node-oauth2-server\node_modules\eslint\lib\cli.js
- node-oauth2-server\node_modules\eslint\bin\eslint.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:957:15)
    at Function.Module._load (internal/modules/cjs/loader.js:840:27)
    at Module.require (internal/modules/cjs/loader.js:1019:19)
    at require (node-oauth2-server\node_modules\v8-compile-cache\v8-compile-cache.js:159:20)
    at Object.<anonymous> (node-oauth2-server\node_modules\eslint\lib\linter\linter.js:27:9)
    at Module._compile (node-oauth2-server\node_modules\v8-compile-cache\v8-compile-cache.js:192:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1153:10)
    at Module.load (internal/modules/cjs/loader.js:977:32)
    at Function.Module._load (internal/modules/cjs/loader.js:877:14)
    at Module.require (internal/modules/cjs/loader.js:1019:19)

Is this a Node 12 issue maybe? I will try it again with Node 14

@jwerre any clue?

jwerre · 2021-10-12T23:36:51Z

hmmm... can you try deleting your node_modules and reinstalling? Also try running ./node_modules/.bin/eslint lib test index.js

HappyZombies · 2021-10-12T23:38:46Z

I did do that, but I will try to delete the entire NPM cache, now I wonder if this a windows issue with version 8 lol.

jwerre · 2021-10-12T23:40:35Z

I did do that, but I will try to delete the entire NPM cache, now I wonder if this a windows issue with version 8 lol.

Ya, sounds like a windows thing... can you spin up a Docker container?

HappyZombies · 2021-10-12T23:58:02Z

Yeah works within Docker...imma make an issue on the ESLint project. I tried searching but get unrelated jest stuff

HappyZombies · 2021-10-12T23:59:47Z

Either way I am bit hesitant now to accept it due to there possibly being an issue with Windows -- but I guess that's a risk we can take.

HappyZombies · 2021-10-13T00:01:33Z

Oh wait let me try this.

Per their docs

Prerequisites: Node.js (^12.22.0, ^14.17.0, or >=16.0.0) built with SSL support. (If you are using an official Node.js distribution, SSL is always built in.)

I am running on Node v12.16.3

I got sidetracked with the Docker stuff lol.

BUT per #34, this means that we need a very specific version of Node now ...and not Node 10 :/

HappyZombies · 2021-10-13T00:06:13Z

Yay version upgrade works! Went on v14.18.1

HappyZombies

@jwerre left a comment on why no-console is off. Really don't understand why this would need to be off. Could seriously disrupt people who use this, and should be highly discouraged.

.eslintrc

oklas · 2021-10-13T15:30:13Z

I actually found a faulty usage of the RegExp 😱

the regex groupings use | as separater (logical or) but this should only be used in this way: (\u0009|\u0020) when used in this way [\u0009|\u0020] it actually accepts a | character.

Maybe we should report this to the original project back?

Really good catch. Of course we should.

jwerre added 2 commits October 11, 2021 09:32

removed jshint added eslint

e650264

merge with development branch

9ebf817

jankapunkt reviewed Oct 11, 2021

View reviewed changes

removed 'no-prototype-builtins' from eslint

0cf3b11

jankapunkt added 2 commits October 12, 2021 10:39

tests validator is tested for all unicode ranges

b4e344d

validator is add missing variable in is.uchar

f57f4c3

jankapunkt added 3 commits October 12, 2021 11:25

tests validator also test for multiple characters

615bd75

eslint enable no-control-regex and disable only in validator

c0e4ef1

tests validator is increase timeout

9775b39

HappyZombies added code quality 🧽 Relating to code quality dependencies 🔌 Pull requests that update a dependency file labels Oct 12, 2021

This was referenced Oct 12, 2021

Remove promisify-any dependency #20

Closed

Remove lodash dependency #23

Closed

jankapunkt requested a review from HappyZombies October 12, 2021 13:34

jwerre commented Oct 12, 2021

View reviewed changes

lib/validator/is.js Show resolved Hide resolved

added chai to is_test.js

391fbef

HappyZombies reviewed Oct 12, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

removed jshint

2e4d14f

HappyZombies reviewed Oct 13, 2021

View reviewed changes

.eslintrc Outdated Show resolved Hide resolved

This was referenced Oct 13, 2021

Bump jshint from 2.13.0 to 2.13.1 #12

Closed

Add TS types #6

Merged

This was referenced Oct 13, 2021

feature: ci with multiple targets #35

Merged

Remove lodash dependency #37

Merged

disallow the use of console

12f0a80

HappyZombies approved these changes Oct 13, 2021

View reviewed changes

HappyZombies merged commit c0196f3 into development Oct 13, 2021

jwerre deleted the lint branch November 21, 2021 14:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaces jshint with eslint #30

Replaces jshint with eslint #30

jwerre commented Oct 11, 2021

jwerre commented Oct 11, 2021

jankapunkt left a comment

jankapunkt Oct 11, 2021

HappyZombies Oct 11, 2021

jwerre Oct 11, 2021

jankapunkt Oct 12, 2021

HappyZombies commented Oct 11, 2021

jwerre commented Oct 11, 2021

jankapunkt commented Oct 12, 2021

jankapunkt commented Oct 12, 2021 •

edited

Loading

jankapunkt commented Oct 12, 2021

jankapunkt commented Oct 12, 2021 •

edited

Loading

HappyZombies commented Oct 12, 2021

jwerre commented Oct 12, 2021

jankapunkt commented Oct 12, 2021

HappyZombies left a comment

HappyZombies commented Oct 12, 2021

HappyZombies commented Oct 12, 2021 •

edited

Loading

jwerre commented Oct 12, 2021

HappyZombies commented Oct 12, 2021

jwerre commented Oct 12, 2021

HappyZombies commented Oct 12, 2021

HappyZombies commented Oct 12, 2021

HappyZombies commented Oct 13, 2021

HappyZombies commented Oct 13, 2021

HappyZombies left a comment

oklas commented Oct 13, 2021

Replaces jshint with eslint #30

Replaces jshint with eslint #30

Conversation

jwerre commented Oct 11, 2021

jwerre commented Oct 11, 2021

jankapunkt left a comment

Choose a reason for hiding this comment

jankapunkt Oct 11, 2021

Choose a reason for hiding this comment

HappyZombies Oct 11, 2021

Choose a reason for hiding this comment

jwerre Oct 11, 2021

Choose a reason for hiding this comment

jankapunkt Oct 12, 2021

Choose a reason for hiding this comment

HappyZombies commented Oct 11, 2021

jwerre commented Oct 11, 2021

jankapunkt commented Oct 12, 2021

jankapunkt commented Oct 12, 2021 • edited Loading

jankapunkt commented Oct 12, 2021

jankapunkt commented Oct 12, 2021 • edited Loading

HappyZombies commented Oct 12, 2021

jwerre commented Oct 12, 2021

jankapunkt commented Oct 12, 2021

HappyZombies left a comment

Choose a reason for hiding this comment

HappyZombies commented Oct 12, 2021

HappyZombies commented Oct 12, 2021 • edited Loading

jwerre commented Oct 12, 2021

HappyZombies commented Oct 12, 2021

jwerre commented Oct 12, 2021

HappyZombies commented Oct 12, 2021

HappyZombies commented Oct 12, 2021

HappyZombies commented Oct 13, 2021

HappyZombies commented Oct 13, 2021

HappyZombies left a comment

Choose a reason for hiding this comment

oklas commented Oct 13, 2021

jankapunkt commented Oct 12, 2021 •

edited

Loading

jankapunkt commented Oct 12, 2021 •

edited

Loading

HappyZombies commented Oct 12, 2021 •

edited

Loading