Skip to content
This repository has been archived by the owner on Aug 21, 2024. It is now read-only.

koa migration for server-core, server, taskserver and instance server #7923

Merged
merged 31 commits into from
May 20, 2023

Conversation

SYBIOTE
Copy link
Member

@SYBIOTE SYBIOTE commented Apr 24, 2023

Summary

migrated server-core to utilize koa over express within feathersjs
actively changed services
file browser
upload asset
scene
for other services, the declaration was changed to use application definition from feathersjs/koa over featherjs/express
package.json is updated to include koa packages as well

References

completes #7802

Checklist

  • If this PR is still a WIP, convert to a draft
  • When this PR is ready, mark it as "Ready for review"
  • ensure all checks pass
  • Changes have been manually QA'd
  • Changes reviewed by at least 2 approved reviewer

QA Steps

List any additional steps required to QA the changes of this PR, as well as any supplemental images or videos.

@SYBIOTE SYBIOTE changed the title koa migration for file browser, upload asset, and scene services koa migration for server-core Apr 24, 2023
@SYBIOTE SYBIOTE changed the title koa migration for server-core koa migration for server-core, server, taskserver and instance server May 12, 2023
@SYBIOTE SYBIOTE assigned SYBIOTE and unassigned SYBIOTE May 12, 2023
package.json Outdated Show resolved Hide resolved
packages/server-core/src/createApp.ts Outdated Show resolved Hide resolved
Copy link
Member

@hanzlamateen hanzlamateen left a comment

Choose a reason for hiding this comment

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

This PR looks good from my side, though it would be good to have @barankyle take a 2nd look of it.

package.json Outdated Show resolved Hide resolved
packages/client/server.js Outdated Show resolved Hide resolved
packages/server-core/package.json Show resolved Hide resolved
packages/server-core/package.json Show resolved Hide resolved
packages/server-core/package.json Show resolved Hide resolved
packages/server-core/package.json Show resolved Hide resolved
@@ -1,7 +1,8 @@
import { Params } from '@feathersjs/feathers'
import { bodyParser, koa } from '@feathersjs/koa'
import Multer from '@koa/multer'
import { Route53RecoveryControlConfig } from 'aws-sdk'
Copy link
Member

Choose a reason for hiding this comment

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

If the aws-sdk upgrade PR gets merged in before this, make sure this is changed to use the new SDK.

Copy link
Member Author

@SYBIOTE SYBIOTE May 19, 2023

Choose a reason for hiding this comment

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

will do if needed

@SYBIOTE
Copy link
Member Author

SYBIOTE commented May 19, 2023

@barankyle , i fixed the issues raised and pushed the code

@SYBIOTE
Copy link
Member Author

SYBIOTE commented May 19, 2023

for the merge conflict, i had to do that to ensure no build error, if its fine without the changes, ill be happy to change it back

@SYBIOTE
Copy link
Member Author

SYBIOTE commented May 19, 2023

@barankyle , added back koa-mount and multer

@SYBIOTE
Copy link
Member Author

SYBIOTE commented May 19, 2023

@barankyle , reverted to use require, do i make any specific changes there?

@barankyle
Copy link
Member

barankyle commented May 19, 2023

@barankyle , reverted to use require, do i make any specific changes there?

There's a couple things that I'm going to push up to this branch.

@barankyle
Copy link
Member

Pushed those, I think that will work. I rebased it around the latest dev, please double-check that I didn't cause any problems when resolving merge conflicts.

@SYBIOTE
Copy link
Member Author

SYBIOTE commented May 19, 2023

thanks a lot, can you tell me, why we do new Koa().koa ? i never saw it any docs, and I'm curious.

@SYBIOTE
Copy link
Member Author

SYBIOTE commented May 19, 2023

and is there a more automatic way like prettier, which works for package.json files? i did go over those files quite a lot of times, missing ascending order because i was looking over them manually doesnt sound like the most efficient way, is there a better way i am missing?

@barankyle
Copy link
Member

barankyle commented May 20, 2023

thanks a lot, can you tell me, why we do new Koa().koa ? i never saw it any docs, and I'm curious.

When I tried to run that file, it complained that Koa was not a constructor. I looked at the structure of Koa and saw there was a function koa on it, so I tried to call that, and it seemed to work.

and is there a more automatic way like prettier, which works for package.json files? i did go over those files quite a lot of times, missing ascending order because i was looking over them manually doesnt sound like the most efficient way, is there a better way i am missing?

For some reason I thought prettier did cover package.json files, but I see it does not (or at least we don't have it configured to handle them). I believe if you run npm install --save <package>, it will put it in the right place alphabetically. Don't worry about the order too much with package.json files, thought.

@barankyle
Copy link
Member

barankyle commented May 20, 2023

There was one last minor issue I fixed, the usage of multer in upload-asset.service.ts had had a file limit of 1 added to it, but that service shouldn't have a limit; there are some situations, like avatar upload, where we upload multiple files at the same time. It's a little confusing since other file upload services have that 1-file limit, but it's intentionally not the same.

It all looks good now.

@barankyle barankyle added this pull request to the merge queue May 20, 2023
Merged via the queue into dev with commit 58d6d36 May 20, 2023
@barankyle barankyle deleted the migrate-koa-to-express branch May 20, 2023 17:47
@SYBIOTE
Copy link
Member Author

SYBIOTE commented May 20, 2023

yay

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants