toptal / haste-server

open source pastebin written in node.js
https://www.toptal.com/developers/hastebin/about
2.92k stars 796 forks source link

Convert haste-server to typescript #420

Closed yusufzmly closed 2 years ago

yusufzmly commented 2 years ago

Description

Converted haste server to typescript, added express.js server.

Next:

These developments are done according to monorepo-approach server example project.

How to test

second way:

yusufzmly commented 2 years ago

I just went through the code, started the app and tried to use redis, so i'll still need to test everything else, but i'm leaving a few comments now. I skipped commenting about things that were present in previous version to not try to fix everything in one pr :)

Apart from inline comments, my overall impression:

  • I think in some places we might have went too far. I think we should stick just to typescript, and do not change the way app configuration works, package manager, packages we use, dir strucutre and so on. Honestly this feels like a new app with some code snippets pasted from v1 :P which may be too big of a change coming from one PR. Reviewing this and testing everything is quite overwhelming, especially because i need to test every single option manually. Its ok to change or introduce new things, but we have to keep in mind this app is open source, so we should consider pros and cons before any major change. And given the amount of changes i'd consider doing it in small steps, just not to miss anything and not to introduce any issues for the users.
  • There is express added without any explanation, was it discussed somewhere else and i missed it? 🤔

@macap So since we are gonna use this monorepo approach for hastebin, i have tried to write haste-server according to it also. https://github.com/toptal/monorepo-research/tree/main/apps/server/ I have used same configs, same structure and express.js because of it. I haven't changed the document stores. I have converted them directly to typescript. Only i did some changes on redis.

We have talked with @filipechagas about this express and structure things etc... Besides, i think converting project to typescript as it is, it would have been very difficult. Maybe, i can create small prs instead of this big pr what do u guys suggest here ? @macap @filipechagas

konstrybakov commented 2 years ago

TBH I tend to agree with @macap. It feels like this PR went far beyond the acceptance criteria of just converting the app to TS. The whole thing about ts is that it's easy to adopt. You would just have 1 extra tsc command before the yarn start.

I think that server adoption should be in another PR. However, seeing that you already did this, this PR is just going to take way longer to review.

yusufzmly commented 2 years ago

@filipechagas let me explain this build:nostatic, maybe we can find another solution. So in normal, yarn build, we have copy:files command which is copying the static files. But this copyFiles package is not working in dockers. So i created another command which build:nostatic like build without static files(not using copyFiles packages) and later in dockers file i copied the static files with COPY static /app/dist/static command.

konstrybakov commented 2 years ago

Also, should we maybe convert the config to TS also? we could utilise the types available

konstrybakov commented 2 years ago

BTW have you checked that datastores are still working?

yusufzmly commented 2 years ago

BTW have you checked that datastores are still working?

@konstrybakov you had duplicated comments somehow. I have deleted them.

About datastores, i have tried file, postgres, redis, memcached. There is amazon, mongo, rethinkdb not tried. I think when we are doing improvement tasks on the stores. We can try all of the stores. What do u think ?

About converting tsconfig, maybe ts or json would be nice also. I can create another task for this. what do u think ?

konstrybakov commented 2 years ago

About converting tsconfig, maybe ts or json would be nice also. I can create another task for this. what do u think ?

Maybe add it to the other task about TS and haste-server?

konstrybakov commented 2 years ago

About the dependencies, sorry, I haven't realized that there is a problem. I think it'd be better, and easier to understand if we had all the necessary dependencies from the start. Sorry for the confusion