Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

Sequelize -> TypeORM #257

Merged
merged 12 commits into from
Dec 6, 2019
Merged

Conversation

Zeko369
Copy link
Member

@Zeko369 Zeko369 commented Nov 21, 2019

  • Update README.md
  • Convert chapterModel to typeORM syntax
  • Setup package.json scripts (ts-node nonsense)
  • Example migrations

@Zeko369 Zeko369 mentioned this pull request Nov 21, 2019
3 tasks
@Zeko369 Zeko369 self-assigned this Nov 21, 2019
@timmyichen
Copy link
Contributor

LGTM! Just need to add / update some docs as well. Thanks for doing this 🎉

@allella
Copy link
Contributor

allella commented Nov 21, 2019

@timmyichen I have a list of small README changes, so I'll update to TypeORM in the docs in a forthcoming PR so you all can focus on the technical stuff.

@Zeko369
Copy link
Member Author

Zeko369 commented Nov 21, 2019

@allella I already changed it in the README

@timmyichen
Copy link
Contributor

@Zeko369 your readme changes revert some things (like contributors), might need to rebase? also, i was thinking we could add some basic docs for running migrations, etc. perhaps in a separate server/README.md

@Zeko369
Copy link
Member Author

Zeko369 commented Nov 21, 2019

Crap, I haven't pulled from upstream, I'll get that back in. Also I'm working on a simple guide for TypeORM features and how to use them, probably will create both Wiki post and server/README.md

@kognise kognise mentioned this pull request Nov 21, 2019
7 tasks
@robertt
Copy link
Contributor

robertt commented Nov 22, 2019

I like this other than how we're using BaseEntity. I find the repository method much easier and simpler to use.

@Zeko369
Copy link
Member Author

Zeko369 commented Nov 22, 2019

@Haxified I think that ActiveRecord pattern is simpler and easier to understand, and since the most used ORM for js/ts is sequelize a lot more people will be familiar with that pattern, also people coming from rails ❤️
I'll create 2 routers, one with repo mode, and one with ActiveRecord and then people can vote

@robertt
Copy link
Contributor

robertt commented Nov 26, 2019

@Zeko369 oops, didnt see this 👍. Fair enough, although I would like to see the 2 routers side by side. The thing i dislike about the repository method is how you need to run database.getRepository to get the repositories. This is easily fixed with dependency injection, and I would definately use it if this project wasn't aimed at beginners, although if you guys think dependency injection would be good then i could def put in a pr.

@Zeko369
Copy link
Member Author

Zeko369 commented Nov 26, 2019

So I created an example of TypeORM "repository" and "ActiveRecord" patterns. My opinion is that we should use ActiveRecord but if anyone has any opinions on why we should use repository mode and not ActiveRecord feel free to comment here

Take a look at server/controllers/chapterController.ts, index vs index_repo
Vote 🚀 for ActiveRecord Pattern and 🎉 for Repository Pattern

@Zeko369
Copy link
Member Author

Zeko369 commented Nov 28, 2019

Will use ActiveRecord pattern since no one voted for RepoPattern

@robertt
Copy link
Contributor

robertt commented Nov 30, 2019

fair enough, i'm happy with both and i've used both a lot

@Zeko369
Copy link
Member Author

Zeko369 commented Dec 6, 2019

I think that we should get this merged so that we can work on converting models from sequelize-ts to typeorm #162 , and I'll create another PR for seeds

Copy link
Member

@vaibhavsingh97 vaibhavsingh97 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@vaibhavsingh97 vaibhavsingh97 left a comment

Choose a reason for hiding this comment

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

LGTM 🌟

@vaibhavsingh97 vaibhavsingh97 merged commit 4c9ef69 into freeCodeCamp:master Dec 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants