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

2769 eliminate ts ignore in admin game setup component #2991

Merged
merged 20 commits into from
Jul 18, 2024

Conversation

lilitkarapetyan
Copy link
Collaborator

@lilitkarapetyan lilitkarapetyan commented Jul 3, 2024

Fixes #2769

🧰 Issue

πŸš€ Overview:

πŸ”— Link to preview

πŸ€” Reason:

πŸ”¨Work carried out:

  • Tests pass

πŸ–₯️ Screenshot

Confirmations

  • I have chosen reviewers for my PR.
  • I have assigned myself to this PR.
  • I have chosen an appropriate label for the PR.
  • I have completed the mandatory sections of this document.
  • I have deleted any unused sections.
  • I confirm that I have checked for required README updates and acted accordingly.

πŸ“ Developer Notes:

@lilitkarapetyan lilitkarapetyan added Technical Debt This doesn't seem right 4p10 Potential to support P10 labels Jul 3, 2024
@lilitkarapetyan lilitkarapetyan requested a review from IanMayo as a code owner July 3, 2024 11:29
@IanMayo IanMayo temporarily deployed to serge-2769-eliminate-ts-nwlbpi July 3, 2024 11:29 Inactive
@IanMayo IanMayo temporarily deployed to serge-2769-eliminate-ts-nwlbpi July 3, 2024 11:42 Inactive
@IanMayo IanMayo temporarily deployed to serge-2769-eliminate-ts-nwlbpi July 3, 2024 12:01 Inactive
@IanMayo IanMayo had a problem deploying to serge-2769-eliminate-ts-nwlbpi July 3, 2024 12:45 Failure
@IanMayo IanMayo had a problem deploying to serge-2769-eliminate-ts-nwlbpi July 4, 2024 10:31 Failure
@Tristina1788
Copy link
Collaborator

@lilitkarapetyan @IanMayo I see the failed tests then I checkout and check from my side. I can run pass without any issue. So I re-run the job and the tests from deployment are also passed. The failed test before maybe from low connection.
But I don't know what I should check in this PR. From UI, seems everything is working as before.

@IanMayo IanMayo had a problem deploying to serge-2769-eliminate-ts-nwlbpi July 9, 2024 08:50 Failure
@@ -155,18 +156,16 @@ export const populateWargameList = () => {
return async (dispatch: WargameDispatch) => {
dispatch(populatingDb(true))

// @ts-ignore
const wargameNames = await wargamesApi.populateWargameList(dispatch)
const wargameNames = await wargamesApi.populateWargameList()
console.warn('now all wargameenams come from server', wargameNames)
Copy link
Contributor

Choose a reason for hiding this comment

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

@lilitkarapetyan - we can probably lose the above debug line

@IanMayo
Copy link
Contributor

IanMayo commented Jul 9, 2024

But I don't know what I should check in this PR. From UI, seems everything is working as before.

Hello @Tristina1788 - this is a code change that should not change the UI functionality. So, everything should work as before - so that's a good sign :-)

@IanMayo IanMayo had a problem deploying to serge-2769-eliminate-ts-nwlbpi July 9, 2024 11:21 Failure
@IanMayo
Copy link
Contributor

IanMayo commented Jul 9, 2024

We have these build errors @lilitkarapetyan. I tried the apply the suggested fix, but I don't think it worked:

       src/ActionsAndReducers/dbMessageTypes/messageTypes_ActionCreators.ts(3,19): error TS7016: Could not find a declaration file for module 'check-types'. '/tmp/build_0e601687/client/node_modules/check-types/src/check-types.js' implicitly has an 'any' type.
         Try `npm i --save-dev @types/check-types` if it exists or add a new declaration (.d.ts) file containing `declare module 'check-types';`
       src/ActionsAndReducers/dbMessages/messages_ActionCreators.ts(4,19): error TS7016: Could not find a declaration file for module 'check-types'. '/tmp/build_0e601687/client/node_modules/check-types/src/check-types.js' implicitly has an 'any' type.
         Try `npm i --save-dev @types/check-types` if it exists or add a new declaration (.d.ts) file containing `declare module 'check-types';`

I'll have to leave it to you, please.

@lilitkarapetyan
Copy link
Collaborator Author

We have these build errors @lilitkarapetyan. I tried the apply the suggested fix, but I don't think it worked:

       src/ActionsAndReducers/dbMessageTypes/messageTypes_ActionCreators.ts(3,19): error TS7016: Could not find a declaration file for module 'check-types'. '/tmp/build_0e601687/client/node_modules/check-types/src/check-types.js' implicitly has an 'any' type.
         Try `npm i --save-dev @types/check-types` if it exists or add a new declaration (.d.ts) file containing `declare module 'check-types';`
       src/ActionsAndReducers/dbMessages/messages_ActionCreators.ts(4,19): error TS7016: Could not find a declaration file for module 'check-types'. '/tmp/build_0e601687/client/node_modules/check-types/src/check-types.js' implicitly has an 'any' type.
         Try `npm i --save-dev @types/check-types` if it exists or add a new declaration (.d.ts) file containing `declare module 'check-types';`

I'll have to leave it to you, please.

I will solove it

@lilitkarapetyan
Copy link
Collaborator Author

Did you run the yarn command ?

@IanMayo
Copy link
Contributor

IanMayo commented Jul 9, 2024

Did you run the yarn command ?

The error comes from the Heroku build, not my local machine. Running yarn install locally fixed my build, but we need the Heroku build to run, too.

@IanMayo IanMayo temporarily deployed to serge-2769-eliminate-ts-nwlbpi July 9, 2024 13:03 Inactive
IanMayo
IanMayo previously approved these changes Jul 9, 2024
@IanMayo
Copy link
Contributor

IanMayo commented Jul 12, 2024

Hello @lilitkarapetyan - this is done, really. But, how about you try to remove some other ts-ignore comments in the code base, too?

@lilitkarapetyan
Copy link
Collaborator Author

Hello @lilitkarapetyan - this is done, really. But, how about you try to remove some other ts-ignore comments in the code base, too?

I Think It's good Idea

@IanMayo
Copy link
Contributor

IanMayo commented Jul 17, 2024

This is good work, thanks @lilitkarapetyan

It seems to cover quite a wide area, so it's worth @Tristina1788 doing some wider testing. @lilitkarapetyan - could you please provide a list of areas of Serge that should be re-tested due to changes in this PR?

@lilitkarapetyan
Copy link
Collaborator Author

This is good work, thanks @lilitkarapetyan

It seems to cover quite a wide area, so it's worth @Tristina1788 doing some wider testing. @lilitkarapetyan - could you please provide a list of areas of Serge that should be re-tested due to changes in this PR?

Hello , I think you could test keeping the Forces Save section in the admin panel and the Chanel tab in the game.
Screenshot (303)
Screenshot (304)

@Tristina1788
Copy link
Collaborator

@IanMayo @lilitkarapetyan Checking all places and I don't see the difference with before.

@IanMayo
Copy link
Contributor

IanMayo commented Jul 18, 2024

I don't see the difference with before.

Good, that's what we're hoping to see 🀣 . Lilit has done some tidying (refactoring) of the source code for Serge, but the test is that nothing has been broken. So, that's what we're looking for @Tristina1788 .

@lilitkarapetyan
Copy link
Collaborator Author

@IanMayo @lilitkarapetyan Checking all places and I don't see the difference with before.

Yes, we need it to work like before.

@Tristina1788
Copy link
Collaborator

I don't see the difference with before.

Good, that's what we're hoping to see 🀣 . Lilit has done some tidying (refactoring) of the source code for Serge, but the test is that nothing has been broken. So, that's what we're looking for @Tristina1788 .

Yes, I understand that. So It's good to go.

@IanMayo IanMayo merged commit 3289dc6 into develop Jul 18, 2024
3 checks passed
@IanMayo IanMayo deleted the 2769_eliminate_ts_ignore_in_AdminGameSetup_Component branch July 18, 2024 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4p10 Potential to support P10 Technical Debt This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eliminate @ts-ignore in AdminGameSetup Component
3 participants