-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
Migrate JS to TS and Test Files to Node.js Native Test runner : Juicy-coupon-bot renovation #25
Conversation
8cad78a
to
3ad7c94
Compare
Just some linting errors leftScreencast.from.2025-03-15.16-01-15.mp4 |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code itself looks pretty good to me 👍
unfortunatly the npm deps can't be installed right now, see pipeline logs.
also this project (in ccontrast to the main juiceshop repo) has a package-lock checked in, please make sure to also commit the changes to it when updating dependencies
package.json
Outdated
"chai": "^4.2.0", | ||
"coveralls": "^3.1.0", | ||
"mocha": "^7.2.0", | ||
"eslint": "^8.57.1", | ||
"eslint-config-standard-with-typescript": "^43.0.1", | ||
"eslint-plugin-import": "^2.28.1", | ||
"eslint-plugin-n": "^17.16.2", | ||
"eslint-plugin-node": "^11.1.0", | ||
"eslint-plugin-promise": "^7.2.1", | ||
"mocha": "^10.4.0", | ||
"nyc": "^15.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there still seem to be some (now) unused testing dependencies in here
Donee 👍 |
.github/workflows/ci.yml
Outdated
@@ -26,7 +26,7 @@ jobs: | |||
node-version: 20 | |||
cache: npm | |||
- name: "Install application" | |||
run: npm install --ignore-scripts | |||
run: npm install --ignore-scripts --legacy-peer-deps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? This repo (in contrast to the main repo) shouldn't have any sec bulbs in it and the dependencies should install cleanly
.eslintrc.cjs
Outdated
parser: '@typescript-eslint/parser', | ||
rules: { | ||
"no-void": "off", // conflicting with recommendation from @typescript-eslint/no-floating-promises | ||
// FIXME warnings below this line need to be checked and fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all these really needed? Or just ported over?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially ported them over, but I have now fixed it. 😅
.eslintrc.cjs
Outdated
], | ||
plugins: ["@typescript-eslint"], | ||
env: { | ||
browser: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are all these unused env enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uhm same i ported these too it's fixed now
package.json
Outdated
"eslint": "^8.57.1", | ||
"eslint-config-standard-with-typescript": "^43.0.1", | ||
"eslint-plugin-import": "^2.28.1", | ||
"eslint-plugin-n": "^17.16.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this pulling in the plugin-n and the plugin-node that the n plugin is replacing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware that the n plugin replaces the node plugin. thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you have another look over the code style formatter? It still doesn't seem to be working/ correct. E.g the code is still using semicolons, this doesn't match the juice-shop repo
.github/workflows/lint-fixer.yml
Outdated
@@ -10,7 +10,7 @@ jobs: | |||
uses: actions/checkout@v2 | |||
- name: Install Linter | |||
run: | | |||
npm install --ignore-scripts | |||
npm install --ignore-scripts --legacy-peer-deps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still leftover
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
i actually misunderstood initially and intentionally added double quotes and semicolon now i have removed that rule |
CI/CD is running... |
I think we're just one |
Files to Migrate ( JS -> TS )
Test Files to Migrate ( Chai , Mocha -> Node.js Native Test Runner )
Linter