[🟦] Migrate server-side code to TypeScript #1573

bkimminich · 2021-01-28T21:54:32Z

Introduce typescript and necesary type packages on backend
Rename all code files from .js to .ts
Augment npm install process to transpile
Change npm start to run server from transpiled JavaScript code

After the above steps typing can be gradually introduced in the codebase.

The text was updated successfully, but these errors were encountered:

paseaf · 2021-01-28T22:16:47Z

I would like to work on it :)

bkimminich · 2021-01-28T22:20:23Z

You are more than welcome to! 👍
I'll assign the issue to you! 🎉

paseaf · 2021-01-29T23:01:01Z

Hi, I have one questions :)

Rename all code files from .js to .ts

Do "all code files" also include test files?

bkimminich · 2021-01-30T00:33:33Z

Absolutely, the tests should also be in TypeScript. In the end all code should be TS, so we can have consistent linting rules and programming patterns across frontend, backend and tests.

paseaf · 2021-01-30T00:42:22Z

Understood! 👍

bkimminich · 2021-02-03T18:41:21Z

Hi @paseaf! How's it going? Did you make some progress? Anything you need help with?

paseaf · 2021-02-03T21:35:45Z

@bkimminich Hi! I've already installed and set up typescript, and am currently in the middle of migrating /test/**/*.js files to .ts. I've created a branch ~~zl/ts-migrate_test~~ ~~zl/ts-migrate_v3~~ ~~zl/ts-migrate_v4~~ zl/ts-migrate_v5.

I do have a question, which scripts should be passed locally for the migration? I'm currently checking test:server, e2e, and frisby for each step, but not sure if they are all I need to care about.

bkimminich · 2021-02-03T21:48:23Z

Wow, that sounds great already! Yeah, those test scripts are exactly the ones that run on server side.

paseaf · 2021-02-09T22:06:56Z

Help wanted
I just changed almost all require/module.exports to typescript syntax import/export to avoid modules being imported to the global scope.
But doing this has caused an error when running npm frisby:

/juice-shop/lib/startup/registerWebsocketEvents.ts:6
import { notifications, challenges } from '../../data/datacache'
^^^^^^

SyntaxError: Cannot use import statement outside a module

I have tried all fixes on Google's first page but none has worked.
Posting here in case anyone knows how to solve it.
Here is the branch: https://github.com/paseaf/juice-shop/tree/zl/ts-migrate_v3

I will try something different to avoid global import if it cannot be solved easily.

J12934 · 2021-02-09T23:09:17Z

@paseaf I'm no expert in this typescript / module stuff, but the problem seems to be that the registerWebsocketEvents itself is loaded via require and can therefore not use the modern import syntax. (https://www.typescriptlang.org/docs/handbook/modules.html#export--and-import--require)

https://github.com/paseaf/juice-shop/blob/zl/ts-migrate_v3/server.ts#L597

I've changed the way to the following and that seems to have fixed the error.
Though I dont think this dynamic import() here is really necessary.
Looks to me as if we can declare that to beforehand.

import('./lib/startup/registerWebsocketEvents').then(
  ({ default: registerWebsocketEvents }) => {
    registerWebsocketEvents(server);
  }
)

paseaf · 2021-02-10T09:10:03Z

@J12934 You are right! 👍 Problem is solved after changing the syntax there.

paseaf · 2021-02-15T22:59:56Z

Hey @bkimminich, I have some questions. 👋

Current Progress [branch: zl/ts-migrate_v4]

Introduced typescript and installed necessary types packages
Renamed almost all .js files to .ts. (with one exception lib/insecurity.js mentioned in question 1 below)
Passed all test:server and frisby tests directly on .ts files with ts-node
Passed e2e tests on .ts files with the same results as in develop branch (Ran 231 of 238 specs
231 specs, 62 failures, 3 pending specs)
Augmented npm install process to transpile
Changed npm start to run server from transpiled JavaScript code
Changed npm run serve to run app with ts-node app.ts

Questions:

Problem: npm run frisby fails once lib/insecurity.js is changed to .ts. I tried to change all module import/export syntax to typescript compliant but couldn't fix it.
Failed suites: test/api/dataExportApiSpec.ts, test/api/basketApiSpec.ts, and test/api/basketApiSpec.ts .
Question: Is it fine to leave lib/insecurity.js as .js and migrate it at a later time?
Question: Should tests also be executed from the compiled directory, or directly on .ts files with ts-node? I assume that the former may be more suitable for production, and the latter for development, but I'm not sure.
Question: Is my e2e test result mentioned above expected?
Question: Are there any other config files that should be updated, for example Gruntfile.js?

bkimminich · 2021-02-16T05:32:08Z

Impressive! 👍 I'll check out your fork in the next days and see if I can reproduce the e2e and insecurity behavior. 👀

bkimminich · 2021-02-17T14:27:48Z

I'm running into an error with npm install for my fresh checkout of your fork:

> juice-shop@12.6.0-SNAPSHOT build:server C:\Data\GitHub\paseaf\juice-shop
> tsc; copyfiles -s "config/**/*" "data/**/*" "encryptionkeys/**/*" "frontend/dist/**/*" "ftp/**/*" "i18n/**/*" "uploads/**/*" "views/**/*" config.schema.yml package.json ./dist/

error TS5023: Unknown compiler option '-s'.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! juice-shop@12.6.0-SNAPSHOT build:server: `tsc; copyfiles -s "config/**/*" "data/**/*" "encryptionkeys/**/*" "frontend/dist/**/*" "ftp/**/*" "i18n/**/*" "uploads/**/*" "views/**/*" config.schema.yml package.json ./dist/`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the juice-shop@12.6.0-SNAPSHOT build:server script.

paseaf · 2021-02-17T17:15:36Z

I'm running into an error with npm install for my fresh checkout of your fork:

> juice-shop@12.6.0-SNAPSHOT build:server C:\Data\GitHub\paseaf\juice-shop
> tsc; copyfiles -s "config/**/*" "data/**/*" "encryptionkeys/**/*" "frontend/dist/**/*" "ftp/**/*" "i18n/**/*" "uploads/**/*" "views/**/*" config.schema.yml package.json ./dist/

error TS5023: Unknown compiler option '-s'.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! juice-shop@12.6.0-SNAPSHOT build:server: `tsc; copyfiles -s "config/**/*" "data/**/*" "encryptionkeys/**/*" "frontend/dist/**/*" "ftp/**/*" "i18n/**/*" "uploads/**/*" "views/**/*" config.schema.yml package.json ./dist/`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the juice-shop@12.6.0-SNAPSHOT build:server script.

Sorry, I didn't know Windows doesn't have ; operator.
I just pushed an update. I tested install, start, frisby, test:server, and e2e scripts on a 64-bit Windows 10 machine with Node v14.15.5. Hope it works now. :D

paseaf · 2021-02-17T18:22:13Z

Impressive! 👍 I'll check out your fork in the next days and see if I can reproduce the e2e and insecurity behavior. 👀

I just realized that I could compare the compiled code with the code before migration to find the problem. I'll check it and see what I can find 👍

bkimminich · 2021-02-17T21:48:34Z

This is what I now get on my Windows 10 with Node 14:

data/datacreator.ts(24,26): error TS7006: Parameter 'file' implicitly has an 'any' type.
data/datacreator.ts(62,29): error TS7031: Binding element 'name' implicitly has an 'any' type.
data/datacreator.ts(62,35): error TS7031: Binding element 'category' implicitly has an 'any' type.
...
<hundreds of errors like this>
...
test/server/verifySpec.ts(303,9): error TS2532: Object is possibly 'undefined'.
test/server/verifySpec.ts(305,32): error TS2532: Object is possibly 'undefined'.
test/server/verifySpec.ts(305,42): error TS2532: Object is possibly 'undefined'.
test/server/verifySpec.ts(305,52): error TS2532: Object is possibly 'undefined'.
test/server/verifySpec.ts(312,9): error TS2532: Object is possibly 'undefined'.
test/server/verifySpec.ts(314,32): error TS2532: Object is possibly 'undefined'.
test/server/verifySpec.ts(314,42): error TS2532: Object is possibly 'undefined'.
test/server/verifySpec.ts(314,52): error TS2532: Object is possibly 'undefined'.
npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! juice-shop@12.6.0-SNAPSHOT build:server: `tsc`
npm ERR! Exit status 2
npm ERR!
npm ERR! Failed at the juice-shop@12.6.0-SNAPSHOT build:server script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

paseaf · 2021-02-17T22:09:40Z

This is what I now get on my Windows 10 with Node 14:

data/datacreator.ts(24,26): error TS7006: Parameter 'file' implicitly has an 'any' type.
data/datacreator.ts(62,29): error TS7031: Binding element 'name' implicitly has an 'any' type.
data/datacreator.ts(62,35): error TS7031: Binding element 'category' implicitly has an 'any' type.
...
<hundreds of errors like this>
...
test/server/verifySpec.ts(303,9): error TS2532: Object is possibly 'undefined'.
test/server/verifySpec.ts(305,32): error TS2532: Object is possibly 'undefined'.
test/server/verifySpec.ts(305,42): error TS2532: Object is possibly 'undefined'.
test/server/verifySpec.ts(305,52): error TS2532: Object is possibly 'undefined'.
test/server/verifySpec.ts(312,9): error TS2532: Object is possibly 'undefined'.
test/server/verifySpec.ts(314,32): error TS2532: Object is possibly 'undefined'.
test/server/verifySpec.ts(314,42): error TS2532: Object is possibly 'undefined'.
test/server/verifySpec.ts(314,52): error TS2532: Object is possibly 'undefined'.
npm ERR! code ELIFECYCLE
npm ERR! errno 2
npm ERR! juice-shop@12.6.0-SNAPSHOT build:server: `tsc`
npm ERR! Exit status 2
npm ERR!
npm ERR! Failed at the juice-shop@12.6.0-SNAPSHOT build:server script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

These are expected type checking errors (because we only renamed the file without really adding any type annotations), and they shouldn't stop Typescript from compiling the code. I thought the current issue was only to rename the files without fixing type errors. But seems we should suppress these errors to avoid confusion.

bkimminich · 2021-02-17T22:16:00Z

I'm totally fine with those warnings showing up, don't get me wrong. I just didn't expect the compiling to fail, and it seems it worked for you.

paseaf · 2021-02-17T22:26:33Z

I'm totally fine with those warnings showing up, don't get me wrong. I just didn't expect the compiling to fail, and it seems it worked for you.

Does npm start work for you?
(I'm asking because I assume I have the same number of errors (Found 2713 errors) as you, but the code is still compiled and runnable on my machine.)

paseaf · 2021-02-17T22:51:29Z

~~I just pushed an update to suppress the npm error. Hope it works now. :)~~
Seems it's still there. I'll try again and let you know.

paseaf · 2021-02-18T11:15:52Z

The error was because npm run <script> will emit an error if the executed script ("build:server": tsc in this situation) emits any.

I found some trick to suppress the error for both Windows and Linux.

- "postinstall": cd frontend && npm install && cd .. && npm run build:frontend && npm run copy-static-files && npm run build:server
+ "postinstall": cd frontend && npm install && cd .. && npm run build:frontend && npm run copy-static-files && (npm run --silent build:server || cd .)

The update is pushed, and I hope you don't see any npm errors for npm install anymore. (Sorry for the mess😆)

~~Another question regarding path resolving:~~ (Solved with a third approach)
My current postinstall script copies /frontend/dist to the ts compiled folder /dist/frontend/dist with copyfiles. But I just realized that a very small portion of fs functions such as fs.copyFileSync() are not using relative path path.resolve(__dirname, 'some/path').
This could lead to operating on /dist/frontend/dist for some code and /frontend/dist for others.

I can see two solutions:

Update all fs operations to use relative paths with path.resolve or path.join (one example: https://github.com/paseaf/juice-shop/commit/581592911ac7d3d7fdb16fcaf7084dd02ee0f668)
Wrap server code to a /src folder, and still compile to /dist. This requires updating all relevant paths. But we don't have to duplicate the /frontend/dist and other files, and it also seems safer to me. But I'm not sure which files should be moved to /src and how doable it is. (I could just try it out though)

I would prefer the 2nd solution but I'm open to any other solutions. :)

bkimminich · 2021-02-18T15:38:46Z

I agree that 2.) sounds a lot cleaner, because fiddling with the frontend can be avoided and it kind of leaves server-side JS files where they are today.

I'm currently testing, compiling works now for me. npm test also passes. I'll check if the other tests work as is and then play with insecurity.js to see if I can reproduce the issues you ran into.

bkimminich · 2021-02-18T16:27:36Z

Frisby tests pass for me with current version, Protractor suite also passed completely for me. So, that's all 👍! I'll check the insecurity.js issues next.

bkimminich · 2021-02-18T17:34:15Z

Okay, I can confirm 10 frisby tests are failing once I rename insecurity.js into .ts. Seems that they all run into 401 errors where other status codes are expected, so I guess the prepared auth token somehow don't work in the tests?

paseaf · 2021-02-19T08:22:14Z

Could be. I will look into it in the next days. 👍

bkimminich · 2021-03-01T18:31:03Z

Hi @paseaf, if you have some new version for us to test locally, you'll ping us, right? You can of course also just open a PR and we'll test it that way (also our CI/CD will have a chance to take a look) but I'm also happy to do some testing rounds before. The last version looked really promising already, so I hope you didn't throw the towel or anything... 😁

paseaf · 2021-03-01T20:37:43Z

Hi @bkimminich, sorry for the delay. I failed to figure out the issue regarding insecurity.ts last week, and then got a bit busy with other stuff (exam season :/) and forgot about the issue. Is it ok if I only move the server-side code into a /src folder for the PR, and maybe someone could help figure out why renaming insecurity breaks tests? Otherwise I would love to continue digging into the insecurity renaming issue, but it could take one or two more weeks...

bkimminich · 2021-03-01T20:42:19Z

I think it‘s perfectly fine to keep insecurity.js and fix it later. The move into src would be really good to have, on the other hand. Good luck with your exams!

paseaf · 2021-03-01T20:51:45Z

I suppose there might be some interactions between the test cases which I don't yet understand. (maybe due to updating the module syntax) Here is something I've tried

Set package.json->jest->testMatch to "<rootDir>/test/api/basket*Spec.ts"
Result on npm run frisby: both basketApiSpec.ts and basketItemApiSpec.ts have failed tests
Set package.json->jest->testMatch to "<rootDir>/test/api/basketItemApiSpec.ts"
Result on npm run frisby: All tests in basketItemApiSpec.ts passed.

Just posting here as a note.

paseaf · 2021-03-01T20:52:43Z

I think it‘s perfectly fine to keep insecurity.js and fix it later. The move into src would be really good to have, on the other hand. Good luck with your exams!

Great! I'll just move things into /src then. Thanks!! :D

paseaf · 2021-03-01T22:17:10Z

Hey @bkimminich, to confirm before moving. 👀
I will move the following files to /src/:

/data/*
/lib/*
/models/*
/routes/*
/app.ts
/server.ts

Am I missing anything, or including any unnecessary files?

bkimminich · 2021-03-03T19:39:38Z

That should be it, yep... Also, very important, please do a rebase with develop before submitting a PR, because there've been quite some changes, including name refactorings which touched dozens of files plus some experimental feature etc.

paseaf · 2021-03-05T23:15:07Z

@bkimminich I just realized that we can avoid copying static files to /dist folder by always using paths relative to the project root. This way both source code and compiled code will access the same static files, and creating a src/ folder seems not necessary anymore.
The PR above uses this approach. 🎉

bkimminich · 2021-03-18T21:40:18Z

Running Juice Shop from source or a packaged distro now works fine on this branch. The only thing having trouble is the Docker image. On CI/CD it fails the smoke tests already, so the build/publish jobs are not even trying to run currently.

bkimminich · 2021-03-19T20:59:41Z

I guess this is why the Docker tests fail from a missing build/ folder... 😁

sh: 1: tsc: not found

bkimminich · 2021-03-19T22:02:06Z

Looks like everything's fine now! Smoke tests (I realized these never checked the unpackaged archive but the main folder ever since, so this was a nice catch along the way), Docker smoke tests, the code snippets are still working from source, ...

... anyone wants to do some more tests or specific checks before we merge into develop and 🍾?

bkimminich · 2021-03-19T22:40:14Z

So, I think all sources should now be in the packaged archives and they should have been in the Dockerfile all along. To be sure I've added a smoke test that retrieves the snippet for one of the backend challenges, which will fail in case the underlying source file is missing.

I guess we really want to merge into develop before tackling the ✖ 1935 problems (0 errors, 1935 warnings), right? Anything else sounds like a trip to rebase-hell to me...

bkimminich · 2021-03-20T10:09:04Z

@paseaf, please send me an email with your address so I can send some "Thank you!"-package* on its way, as this was a) your first and b) a veeery sophisticated contribution (that even we from core team didn't dare touch until now ourselves...)!!!

⭐⭐⭐⭐⭐! 😀

(*Depending on where you are located I might send you some gift card instead for getting a Juice Shop shirt printed locally or something like that.)

paseaf · 2021-03-20T10:09:52Z

I guess we really want to merge into develop before tackling the ✖ 1935 problems (0 errors, 1935 warnings), right? Anything else sounds like a trip to rebase-hell to me...

I agree..
Another solution could be not renaming all .js files to .ts at once for this merge. We can rename files one by one in the future, each time solving all type warnings in the file. This way should be cleaner, because the warnings only show up in .ts files.
I could create a new branch from ts-backend and try it out. (so that we can compare)

bkimminich · 2021-03-20T10:31:10Z

I just merged it into develop as is. Docker images have been published successfully, Heroku staging environment works. All good. Now it's just this weird insecurity.js issue and gradually changing the code base to actual TS. Can all be done in parallel to normal development afaik.

paseaf · 2021-03-20T10:38:29Z

Great!!
I'm surprised there will be a package. That's very sweet of you. Thank you 😊

J12934 · 2021-03-23T10:12:55Z

Hey ho 👋
Whats our plan on moving forward on migrating the code to more idiomatic typescript?

I think there are some parts of the code base which are "harder" problem, e.g. the models but would provide massive benefits if we could figure out how to properly type them. Do we want to open up separate tickets for the different parts of the code base, so that we can work on them in a distributed manner? Or are there better ways to do it?

bkimminich · 2021-03-23T10:38:32Z

I alteady started fixing some of the easier linter errors, like using template strings over concatenation of non-string types with strings.

Not sure if this approach will work out, as there are probably not so many "easy" issues to address.

bkimminich · 2021-03-24T22:14:51Z

We're down to this list of ESLint complaints:

        '@typescript-eslint/no-misused-promises': 'off', // 97
        '@typescript-eslint/explicit-function-return-type': 'off', // 132
        '@typescript-eslint/restrict-template-expressions': 'off', // 259
        '@typescript-eslint/strict-boolean-expressions': 'off', // 388
        '@typescript-eslint/restrict-plus-operands': 'off', // 496
        '@typescript-eslint/no-var-requires': 'off' // 502

I would have hoped some a auto-fixable - like using template strings over string concatenation - but it seems none of them is, unfortunately.

github-actions · 2022-03-25T00:08:28Z

This thread has been automatically locked because it has not had recent activity after it was closed. 🔒 Please open a new issue for regressions or related bugs.

bkimminich added help wanted technical debt build process installation labels Jan 28, 2021

bkimminich assigned paseaf Jan 28, 2021

paseaf mentioned this issue Mar 5, 2021

[WIP] Migrate server-side code to TypeScript #1598

Merged

5 tasks

bkimminich closed this as completed Mar 20, 2021

github-actions bot locked and limited conversation to collaborators Mar 25, 2022

[🟦] Migrate server-side code to TypeScript #1573

[🟦] Migrate server-side code to TypeScript #1573

Comments

bkimminich commented Jan 28, 2021 • edited Loading

paseaf commented Jan 28, 2021

bkimminich commented Jan 28, 2021

paseaf commented Jan 29, 2021

bkimminich commented Jan 30, 2021

paseaf commented Jan 30, 2021

bkimminich commented Feb 3, 2021

paseaf commented Feb 3, 2021 • edited Loading

bkimminich commented Feb 3, 2021

paseaf commented Feb 9, 2021 • edited Loading

J12934 commented Feb 9, 2021

paseaf commented Feb 10, 2021

paseaf commented Feb 15, 2021 • edited Loading

bkimminich commented Feb 16, 2021

bkimminich commented Feb 17, 2021

paseaf commented Feb 17, 2021

paseaf commented Feb 17, 2021

bkimminich commented Feb 17, 2021

paseaf commented Feb 17, 2021

bkimminich commented Feb 17, 2021

paseaf commented Feb 17, 2021 • edited Loading

paseaf commented Feb 17, 2021 • edited Loading

paseaf commented Feb 18, 2021 • edited Loading

bkimminich commented Feb 18, 2021

bkimminich commented Feb 18, 2021

bkimminich commented Feb 18, 2021

paseaf commented Feb 19, 2021

bkimminich commented Mar 1, 2021

paseaf commented Mar 1, 2021

bkimminich commented Mar 1, 2021

paseaf commented Mar 1, 2021 • edited Loading

paseaf commented Mar 1, 2021

paseaf commented Mar 1, 2021

bkimminich commented Mar 3, 2021 • edited Loading

paseaf commented Mar 5, 2021 • edited Loading

bkimminich commented Mar 18, 2021

bkimminich commented Mar 19, 2021

bkimminich commented Mar 19, 2021

bkimminich commented Mar 19, 2021

bkimminich commented Mar 20, 2021

paseaf commented Mar 20, 2021 • edited Loading

bkimminich commented Mar 20, 2021

paseaf commented Mar 20, 2021

J12934 commented Mar 23, 2021

bkimminich commented Mar 23, 2021

bkimminich commented Mar 24, 2021

github-actions bot commented Mar 25, 2022

bkimminich commented Jan 28, 2021 •

edited

Loading

paseaf commented Feb 3, 2021 •

edited

Loading

paseaf commented Feb 9, 2021 •

edited

Loading

paseaf commented Feb 15, 2021 •

edited

Loading

paseaf commented Feb 17, 2021 •

edited

Loading

paseaf commented Feb 17, 2021 •

edited

Loading

paseaf commented Feb 18, 2021 •

edited

Loading

paseaf commented Mar 1, 2021 •

edited

Loading

bkimminich commented Mar 3, 2021 •

edited

Loading

paseaf commented Mar 5, 2021 •

edited

Loading

paseaf commented Mar 20, 2021 •

edited

Loading