-
Notifications
You must be signed in to change notification settings - Fork 36
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
Server Typescript Migration #65
base: dev
Are you sure you want to change the base?
Conversation
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.
First round of review by me. I will likely go over it again, it's late and I've done a lot of stuff in other repos as well. Going to ping @CaramelKat for her review as well.
Overall looks pretty good 👍 just a few nitpicks and some actual issues/suggestions/clarifications.
I will also note: in the checklist you say you haven't tested these changes. Is there a reason for that? Ideally this should all be tested, on console, before being merged.
Also; this has made me realize that this is a decently sized repository with a lot of moving parts, especially when trying to juggle support for 3 different platforms at the same time. It's resulted in some pretty janky code, dead code left over that isn't clear if it can be removed or not, and an overall not great experience working in the code base.
This is especially present in the first question asked here:
Are these all one project or should they really have their own separate eslint configs? One reason I'm thinking this is that they all appear to use a variety of globals that I'm not sure are applicable to every one of the projects.
This is a valid question. It feels like 3 different projects shoved into one box, and it's not going super well imo. I unfortunately don't have an answer at this time since currently they are one project, but I'd like to possibly change that.
What version of ES should we be aiming for in each of these projects?
For the consoles, I'd say ES5 at the latest. NWFX targets ES3, but I didn't have any issues when using ES5 in some testing. That being said, NWFX is a simpler code base than this.
For the browser, latest always imo.
We should really consider a better way to structure this project, or just separate it out entirely into 3 different ones and bite the bullet of some duplicated backend code. We already duplicate some backend logic as it is, since the miiverse-api server has pretty similar logic to some of these routes.
Yeah, this absolutely needs testing. I am happy to do that but I don't know the best way to set this up for local testing - especially on console |
All you'd need in this case is:
@MatthewL246's Docker project can set this kind of thing up super easily https://github.com/MatthewL246/pretendo-docker In my case when doing local testing all I do is:
However there's a variety of ways to connect locally. I mostly do it this way so that I can avoid setting up SSL certificates just for local testing (redirect from https (Nintendo) to http (local)). You'll then need to create some server documents on the account server for the friends server (NEX) and Miiverse (service), and make some local PNIDs on your instance for testing. That being said that only really works for connecting to most services locally. Apps like the eShop, NNID account settings, and importantly Miiverse are actually all just browser apps in a web view. They roll their own SSL, rather than using the system SSL, so they need additional patches. You'll need a custom build of https://github.com/PretendoNetwork/Inkay for this for 2 reasons:
I'll have to ping @CaramelKat about the exact steps for this (she uses a custom build iirc). This should probably all be added to our developer documentation at some point. |
Hello! It's going to be a little bit of work to get my project working with this PR, since I use some patches to the source code and they'll all be invalid now with all the file renames. I can get that done pretty soon today. The readme has full instructions for setup and connecting to your own server, including the custom Inkay patches Jon mentioned. |
To be clear my suggestion was not to use your project for this PR, as in the juxtaposition-ui server. The suggestion was to use your project to get an instance of the other servers up (account and friends) without much hassle |
I'm not sure I'm understanding your proposal. I can see that Juxt depends on account, friends, Mongo, and Redis. Are you proposing running Juxt on the host and connecting it to the already-set-up containerized services with port forwarding? That doesn't really make sense to me? Nginx and mitmproxy run inside containers too. I am aware though that my project is not great for actively developing on the servers, mainly because using patches messes up the diffs, but it should be fine just for testing in this case. |
I'm not really sure where the disconnect here is? I thought the proposal was pretty straight forward
I'm not sure why? It makes a ton of sense to me. If all that's needed is to test the juxtaposition-ui server, then there's no real need to spend time setting up all the rest of the infrastructure yourself. That's the entire point of Docker containers like these, to quickly spin up instances of the servers without needing to manually setup the environment. Working in this way makes a ton of sense, imo. There's no need to set up the environment to run servers you aren't actually working on, when all you need is for them to be available locally. Mixing containers and non-containers is extremely common in my experience when doing dev work, I very often spin up containers for certain dependencies (especially those which require specialized setups to work, such as Cassandra and SSSL, the latter of which requires it's own custom build of nginx), and then just work on and run what I'm actually working on in the host. This is a very common way of working in my experience.
It seems I may have been mistaken about the usefulness of your project with relation to development work, then. I was under the impression that your project would allow for spinning up quick instances of servers for this very use case, where you may not necessarily want to setup the full server yourself but you need a local instance of it running for what you are working on. |
I see. There was a disconnect in how we thought about developing the services using my project. I had assumed that one would work on everything as containerized all the time, including what they're currently working on (commonly known as "dev containers"). Probably using a bind mount instead of rebuilding the containers every time? I don't know which strategy is better, we can discuss that another time.
Edit 2: It's definitely possible to set this up as you described. The trick is setting the Nginx upstream of the relevant server to |
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.
Overall this works great, using the couple changes I highlighted here I was able to get the server running locally with success using the desktop, Wii U and 3DS clients. The biggest issue left here is building. Right now, the dist folder structure looks like this:
\dist
|--\config.json
|--\src
|- everything else...
This prevents tsc-alias from actually working correctly, since it can't find the files it needs to fix. This also causes issues with the current package.json, since it won't be able to find the right place to start from (which is a small problem, that can easily be updated). We are also missing the views directory, but that can be easily copied, like we already do on the account server here.
The bigger issue is until we move away from using the config.json file, it will create this structure with the src folder. A temp solution would be to move the config file into the src directory and update the imports. This would block the creation of the src folder and allow tsc-alias to run correctly, though it's less than ideal. Personally, I think that would be okay just to get us over the finish line so we can move to a setup like we have in the account server with .env for our configs, but I personally think that would be out of scope for this PR. I'd like @jonbarrow's input though, since I know this is one of the larger repositories that are getting moved and I know that a bigger rewrite is likely going to be happening regardless here in the not too distant future.
Other than that, great job. For not having an environment to test this in before, this was remarkably water tight in my own local testing. I hope to have a better guide for setting up local testing this weekend, but it depends on how things go.
Since I've confirmed this is working in test environments, my vote is to move this into dev to keep testing and stop blocking additional development. @jonbarrow since you still had an open review on this, do you have any objections to that? |
Changes:
This migrates all serverside JavaScript files to TypeScript. I've tried to change as little as possible, but there are some changes to make types work out and some changes to code that must have been wrong / broken.
This also updates some eslint and TypeScript settings, and resolves some eslint errors, also adds
dist
to.gitignore
I have removed the
webfiles
projects from the scope of TypeScript compilation for now as they should likely be their own compiled projectseslint
webfiles
directoryoffset
as askip
parameter to the mongo query, and updates the link sent as a response so that pagination should work properly. I assume this is what was intendedsrc/utils.js
was usingsharp
without importing it, so I've added an import for thatI have also disabled eslint for all projects in the
webfiles
directory temporarily as the eslint settings that were being applied were likely inappropriate. I have also included a separate eslint configuration file for these projects with an initial attempt as a sensible configuration. However, there are some questions before I can finish this:TypeScript
webfiles
sourcesTasks