-
-
Notifications
You must be signed in to change notification settings - Fork 22.2k
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
feat: Setup for option to run as express server #2447
feat: Setup for option to run as express server #2447
Conversation
@Otard95 is attempting to deploy a commit to the github readme stats Team on Vercel. A member of the Team first needs to authorize it. |
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.
Looks good! Do note, however, that #2178 and #2409 will add subdirectories. That is, /api/wild/pin
, or /api/status/up
, for example. It would be great if you could modify your code such that these endpoints are also included.
You can do this in two ways:
- Manually, by recursing through the
wild
andstatus
subdirectories - Programatically
I'm not exactly sure how you can do the second method, but fs
might have some options to recurse through subdirectories.
Thanks a lot for the PR, and it would be great if you could incorporate these changes!
@rickstaa please feel free to comment if you have anything to add.
Codecov ReportBase: 96.75% // Head: 96.75% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #2447 +/- ##
=======================================
Coverage 96.75% 96.75%
=======================================
Files 22 22
Lines 3883 3883
Branches 329 329
=======================================
Hits 3757 3757
Misses 124 124
Partials 2 2 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
With documentation and a simple docker file we are good for a merge 💪. Maybe add the docker in the #1975 section while pointing out that the Vercel deployment is the currently recommended way. Maybe with a (recommended method)
suffix behind the Vercel deployment.
I'm thinking of adding options to use TLS as well as some other security options. Though I could add that as a separate PR. |
Great! You can put it in this PR, and I will look at it. 👍 |
@Otard95 thanks for the PR. As much as I would like to provide out of the box solution for multiple deployments via serverless/stateful means, for us it doesn't make sense to maintain the code to run GRS as express server. and have Dockersetup while we are not even going to use it. For now if you want to deploy on your own express server I will recommend forking GRS and making the change on your fork.
There will be other complexities/edge cases to handle if we support express out of the box. I think moving forward a better approach is to create a reusable npm package for GRS, so that you can just install that package and run it where ever you like with your own infra (be it express or vercel or github-action) Publishing GRS as NPM package will also help in #2179 |
@anuraghazra publishing as an NPM package should be fairly easy. GRS uses ESM packages though, and I haven't worked much with ESM, so I don't know how they work with NPM. As far as I understand, we need to convert to CommonJS to be able to push as an NPM package. If we figure that out, then I can work on a PR that releases GRS as an NPM package. |
Ideally we should keep ESM, GRS was commonjs before we changed it to ESM later on to improve DX. If we just support ESM it's better to do it via TypeScript compiler, this might help |
Hey @anuraghazra!
I totally understand! I created the fork for myself initially and thought I could just as well setup this PR just in case it was withing scope, as I was unsure of originally.
And I'd agree that creating a NPM package seems far more flexible, separation of concerns and all that. One little site-note though, I'd add a section in the |
Also, I'll be continuing to work on my fork, but you can of course close this PR if you'd like. Or keep it active, just in case or for anyone who might find it later, that's up to you. |
86aafe8
to
8bc69e7
Compare
I built the docker image from your fork. After a few hour's debug. I believe your Dockerfile made a mistake about copying file. The relative paths to index.js and other *.js from index.js are disrupted. So, I made some change
Now, with build cmd |
Hey @JamesFlare1212. I did miss the
As far as I can tell, your example is identical to what I have, or maybe I'm just missing somthing. I'm also interested in why you decided to add |
Maintainability and performance boosting will be hard for us when the majority of our users use Vercel, and having these extra files and dependencies will only slow down deployment most of the time. Thanks a lot for your PR, and I'm sorry we weren't able to merge it. I've created #2525 as a compromise between scalability and user convenience, and hopefully that gets merged. Thanks for understanding! |
I totally understand @Zo-Bro-23. And #2525 seems like a good compromise, hope it gets merged 😄 |
Personally, I'd prefer to just spin this up in a Docker container on my own server instead of using Vercel.
So here's a simple solution, adding express as an optional dependency, and spinning up a server and dynamically using the files in the
api
folder to generate the routes.I'm not sure if this is within the scope of this repo, so for now I've omitted documentations and such. Should this become something you'd actually like to merge, that can be added later.
EDIT:
I should also mention that I have no experience with Vercel, so for all I know this may break something on that front.