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.

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

RD-1291 Introduced TS to backend #1575

Merged
merged 34 commits into from
Sep 7, 2021
Merged

RD-1291 Introduced TS to backend #1575

merged 34 commits into from
Sep 7, 2021

Conversation

kubama
Copy link
Contributor

@kubama kubama commented Sep 1, 2021

Description

Although this PR is massive it's not as bad as it looks. All changes made to production and test code is just migration to TS modules, so the set of applied changes is pretty consistent across these files. Additionally there are some required changes around modules mocking in jest tests - I can elaborate about these on our weekly, as there are some gotchas.
Core changes are done in just a few files - package.json, tsconfig.json and .eslintrc.json.
Unfortunately, a few files that were properly renamed appear as deleted/added after committing 🙁.

Screenshots / Videos

N/A

Checklist

  • My code follows the UI Code Style.
  • I verified that all tests and checks have passed.
  • I verified if that change requires a documentation update.
  • I added proper labels to this PR.

Tests

https://jenkins.cloudify.co/view/UI/job/Stage-UI-System-Test/1185/

Documentation

N/A

@kubama kubama added the internal Pull request with internal change (e.g. refactoring, CI changes, documentation update) label Sep 1, 2021
@kubama kubama requested review from qooban and stepek September 1, 2021 09:45
@stepek
Copy link
Contributor

stepek commented Sep 1, 2021

@kubama can you split #d447df2 to smaller commits?

@kubama
Copy link
Contributor Author

kubama commented Sep 1, 2021

@kubama can you split #d447df2 to smaller commits?

I could, but I'm not sure it would be of much help. As I stated in the description, the core changes are limited to just a few json files. All the changes to the actual source files are just conversion to TS modules. There are no changes to a single file that could be split across different commits.

@qooban
Copy link
Contributor

qooban commented Sep 1, 2021

Oh, quite big change. It'll take some time to review.

Unfortunately, a few files that were properly renamed appear as deleted/added after committing slightly_frowning_face.

Having one commit containing both - rename and migration-related changes - won't help. AFAIR from what Grzegorz found is that GitHub works better if you do the rename in one commit and the changes inside the file in the other commit.

https://jenkins.cloudify.co/view/UI/job/Stage-UI-System-Test/1177/

I see there's a problem with starting the backend:

14:15:02  > stage-backend@6.3.0--.dev1 wait-on-server /opt/cloudify-stage/backend
14:15:02  > wait-on tcp:localhost:8088 --timeout 5000 && echo 'Stage backend is up!'
14:15:02  
14:15:07  Error: Timeout
14:15:07      at Timeout._onTimeout (/opt/cloudify-stage/backend/node_modules/wait-on/lib/wait-on.js:113:10)
14:15:07      at listOnTimeout (internal/timers.js:554:17)
14:15:07      at processTimers (internal/timers.js:497:7)
14:15:07  npm ERR! code ELIFECYCLE
14:15:07  npm ERR! errno 1
14:15:07  npm ERR! stage-backend@6.3.0--.dev1 wait-on-server: `wait-on tcp:localhost:8088 --timeout 5000 && echo 'Stage backend is up!'`
14:15:07  npm ERR! Exit status 1
14:15:07  npm ERR! 
14:15:07  npm ERR! Failed at the stage-backend@6.3.0--.dev1 wait-on-server script.
14:15:07  npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

Do we need any update in terms of installed packages on the server to execute ts-node? Maybe it should be installed globally? (npm i -g ts-node)

Other questions before I jump into the review (possibly tomorrow):

  1. Did you manage to change all the files to .ts?
  2. How many of them are already fully converted?
  3. Could you provide some guidelines on how to do similar migration in Composer? (it'd be very much appreciated)

@kubama
Copy link
Contributor Author

kubama commented Sep 1, 2021

Having one commit containing both - rename and migration-related changes - won't help. AFAIR from what Grzegorz found is that GitHub works better if you do the rename in one commit and the changes inside the file in the other commit.

Yes, I didn't expect that as I did just minor updates to the renamed files. In fact git respected renames for most of the files.

I see there's a problem with starting the backend:

14:15:02  > stage-backend@6.3.0--.dev1 wait-on-server /opt/cloudify-stage/backend
14:15:02  > wait-on tcp:localhost:8088 --timeout 5000 && echo 'Stage backend is up!'
14:15:02  
14:15:07  Error: Timeout
14:15:07      at Timeout._onTimeout (/opt/cloudify-stage/backend/node_modules/wait-on/lib/wait-on.js:113:10)
14:15:07      at listOnTimeout (internal/timers.js:554:17)
14:15:07      at processTimers (internal/timers.js:497:7)
14:15:07  npm ERR! code ELIFECYCLE
14:15:07  npm ERR! errno 1
14:15:07  npm ERR! stage-backend@6.3.0--.dev1 wait-on-server: `wait-on tcp:localhost:8088 --timeout 5000 && echo 'Stage backend is up!'`
14:15:07  npm ERR! Exit status 1
14:15:07  npm ERR! 
14:15:07  npm ERR! Failed at the stage-backend@6.3.0--.dev1 wait-on-server script.
14:15:07  npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

Do we need any update in terms of installed packages on the server to execute ts-node? Maybe it should be installed globally? (npm i -g ts-node)

Nothing extra needs to be done.

The issue here seems to be just the timeout - the backend now starts slightly longer, which is not unexpected. I increased the timeout and submitted new job (link in description updated).

  1. Did you manage to change all the files to .ts?

No, db migrations and sequelize models (also used by migrations) are not changed. These are not directly imported, and the logic that handles them is part of cloudify-ui-common and used by composer as well - if we want these to be converted as well it will be best to first migrate composer backend to TS and then handle it as part of a separate ticket.

  1. How many of them are already fully converted?

6 production source files and 3 test files are fully migrated.

  1. Could you provide some guidelines on how to do similar migration in Composer? (it'd be very much appreciated)

Here are the steps in short:

  1. Install ts-node and typescript as production dependencies
  2. Rename all source files (except migrations and sequelize models) and test files to .ts
  3. Replace all node occurrences with ts-node and all direct references to js files with their corresponding ts files
  4. Look for all wildcards referencing js files and change them so they referenced both js and ts files
  5. Convert all renamed files to valid TS modules (import/export)
  6. Test files that used require inside functions will need to have module mocking updated:
  • replace all jest.doMock calls with jest.mock
  • move all jest.mock calls to top level
  • make sure all required jest.mock calls are done directly in the test file (not in an imported file)
  1. Align eslint configuration

@kubama
Copy link
Contributor Author

kubama commented Sep 1, 2021

There is one system test failure - looks like an issue with widget backend. I'll investigate.

Copy link
Contributor

@qooban qooban left a comment

Choose a reason for hiding this comment

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

I did partial review (78/124 files) so far. Will continue later, probably tomorrow (due to the planned meetings). Added some comments.

Very hard to review files which are detected as deleted/added. It's not the time right now, but I'd suggest for the future to consider splitting such huge PR into smaller pieces. I suspect it must be possible to have it done within few PRs, one building on top of the other.

backend/.eslintrc.json Show resolved Hide resolved
backend/babel.config.js Show resolved Hide resolved
backend/package.json Outdated Show resolved Hide resolved
backend/package.json Show resolved Hide resolved
backend/serverSettings.ts Outdated Show resolved Hide resolved
Comment on lines -8 to -14
"settings": {
"import/resolver": {
"jest": {
"jestConfigFile": "jest.config.js"
}
}
},
Copy link
Contributor

@qooban qooban Sep 2, 2021

Choose a reason for hiding this comment

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

What was the reason behind removing it?

Did you have problems with resolving .ts files? (if so, see my comment: https://github.com/cloudify-cosmo/cloudify-blueprint-composer/pull/921/files#r700138908)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I couldn't make it working.
In the end there are two rules that check the same thing: node/no-missing-import and import/no-unresolved. There is no point in having both, so I decided to disable the second one (thus removing the setting).

Comment on lines +5 to +6
import appConfig from '../../../conf/config.json';
import userConfig from '../../../conf/userConfig.json';
Copy link
Contributor

Choose a reason for hiding this comment

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

If we bring back jest import resolver, we can avoid using such paths.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really - the import resolver impacts eslint only. Here the issue was tsc not resolving the modules as they were originally specified

backend/tsconfig.json Show resolved Hide resolved
backend/tsconfig.json Show resolved Hide resolved
devServer.js Outdated Show resolved Hide resolved
@kubama
Copy link
Contributor Author

kubama commented Sep 2, 2021

It's not the time right now, but I'd suggest for the future to consider splitting such huge PR into smaller pieces. I suspect it must be possible to have it done within few PRs, one building on top of the other.

Thinking of it now I think it could be split into first introducing TS support without renaming any of the source files, and then gradually renaming the files and applying required changes (TS modules). Unfortunately I wasn't upfront aware of the scale of the changes that were required. I hope it's a lesson learned for composer backend work.

@kubama kubama requested a review from stepek September 2, 2021 11:15
stepek
stepek previously approved these changes Sep 3, 2021
Copy link
Contributor

@stepek stepek left a comment

Choose a reason for hiding this comment

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

any new comments from me.

Copy link
Contributor

@qooban qooban left a comment

Choose a reason for hiding this comment

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

Second part of the review. 104/124 files reviewed, so a bit closer to finish.

backend/package.json Show resolved Hide resolved
backend/package.json Show resolved Hide resolved
backend/package.json Show resolved Hide resolved
backend/samlSetup.ts Show resolved Hide resolved
backend/handler/services/LoggerService.ts Show resolved Hide resolved
backend/handler/services/ManagerService.ts Show resolved Hide resolved
backend/test/migration.test.ts Show resolved Hide resolved
backend/test/handlers/ArchiveHelper.test.ts Outdated Show resolved Hide resolved
backend/test/handlers/ArchiveHelper.test.ts Show resolved Hide resolved
Copy link
Contributor

@qooban qooban left a comment

Choose a reason for hiding this comment

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

OK, I'm done with the review. I have no more additional comments. I didn't manage to review all the files with the same attention to detail, but I believe that we have some time until the next release to detect any potential problems.

backend/config.ts Show resolved Hide resolved
@kubama kubama requested a review from qooban September 6, 2021 11:22
Copy link
Contributor

@qooban qooban left a comment

Choose a reason for hiding this comment

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

Added two small comments.

package.json Outdated Show resolved Hide resolved
devServer.ts Outdated Show resolved Hide resolved
@kubama kubama requested a review from qooban September 7, 2021 07:54
Copy link
Contributor

@qooban qooban left a comment

Choose a reason for hiding this comment

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

I appreciate hard work on that migration 👍🏻

Could you summarize leftovers (migrations? DB models? any other?) and possibly add tickets for them? (labelled with UI stage typescript)

@kubama kubama merged commit 1b7bf59 into master Sep 7, 2021
@kubama kubama deleted the RD-1291-backend-ts branch September 7, 2021 09:37
@kubama
Copy link
Contributor Author

kubama commented Sep 7, 2021

@qooban Created RD-3119 and RD-3120

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Pull request with internal change (e.g. refactoring, CI changes, documentation update)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants