olmps / memo

Memo is an open-source, programming-oriented spaced repetition software (SRS) written in Flutter.
BSD 3-Clause "New" or "Revised" License
1.83k stars 159 forks source link

[PROPOSAL] Firebase Folders Refactoring #255

Closed ggirotto closed 2 years ago

ggirotto commented 2 years ago

Propose a minor refactoring in the folders structure:

matuella commented 2 years ago

Could you please elaborate on each refactor and why these are an improvement to the current architecture?

ggirotto commented 2 years ago

Could you please elaborate on each refactor and why these are an improvement to the current architecture?

You mentioned previously that you couldn't make functions/ be outside the src/ folder, since it relates to firebase-functions and not with the codebase source code itself, this is the first issue addressed by the changes above.

The second benefit is to don't have multiple package.json files in the project, in which some dependencies like firebase-functions was replicated. This could cause problems when updating such dependencies in the root package.json but not in the onde present in src/core.

The dist/ change is more a personal preference than anything, it could be placed in src/dist as well, this one is a w/e for me.

Basically these are the improvements that are addressed by this refactoring. Now we separate which are firebase functions dependencies (in this case we have only index.ts but we can easily add some firestore-seed as we have in other projects) from the project source files. Let me know your thoughts about them.

matuella commented 2 years ago

You mentioned previously that you couldn't make functions/ be outside the src/ folder, since it relates to firebase-functions and not with the codebase source code itself, this is the first issue addressed by the changes above.

I don't quite remember that well, but I guess that was before the new structure, which makes sense, IMO, as it is. Having it all under src/ makes sure that any sub-project that wants to use core/ is placed under there, such as ec2 or aws-lambda. If we'd move functions to the root project, I think src/ strategy becomes invalidated, as the same rationale that is used for functions/ is the same of scripts/ and other future projects, like mentioned before. I don't see benefits why moving functions/ is positive for us, given that how we planned on src/ being our "code" entry-point.

The second benefit is to don't have multiple package.json files in the project, in which some dependencies like firebase-functions was replicated. This could cause problems when updating such dependencies in the root package.json but not in the onde present in src/core.

The problem with this is that you don't know where each dependency depends on what. Having smaller package.json with specific dependencies of each project (like /functions having only the necessary firebase dependencies to interface all of our core stuff and serve it through the Firebase services).

The dist/ change is more a personal preference than anything, it could be placed in src/dist as well, this one is a w/e for me.

I'm not against it, just wanting to know exactly the why, as I see no difference from what was before.

ggirotto commented 2 years ago

I don't quite remember that well, but I guess that was before the new structure, which makes sense, IMO, as it is.

I don't remember exactly when you changed your mind about it, but I remember that you said to me that we faced so many issues trying to move the functions/ folder outside that you gave up.

Having it all under src/ makes sure that any sub-project that wants to use core/ is placed under there, such as ec2 or aws-lambda. If we'd move functions to the root project, I think src/ strategy becomes invalidated, as the same rationale that is used for functions/ is the same of scripts/ and other future projects, like mentioned before. I don't see benefits why moving functions/ is positive for us, given that how we planned on src/ being our "code" entry-point.

Honestly, in my personal opinion both structures doesn't bring any kind of benefit, they're just different styles. I could argue that functions/ is not code, it's only a necessary structure to expose to Firebase Functions environment the entry point to the actual code, such as we would do with aws-lambda or any other third party provider, but this is a completely w/e to me honestly, I can move functions to src folder again as it was before, I just focused on trying to move it because I remember that you tried and couldn't made it. I thought it was still a problem in the structure.

The problem with this is that you don't know where each dependency depends on what.

But this honestly doesn't matter. We can infer that if a dependency is placed under dependencies in package.json, it's obviously used by the "main" project, since it must be deployed to the Firebase Functions env, but if a devDependency is being used by a script or test, it actually matter to us? In my opinion it doesn't. And making a bridge if the next comment that you made:

Having smaller package.json with specific dependencies of each project (like /functions having only the necessary firebase dependencies to interface all of our core stuff and serve it through the Firebase services)

And connecting with the point that I made in the PR description: splitting package.json may cause inconsistencies around the project. Imagine if we had a package.json under scripts/ folder and another one under core/ foder. If scripts uses any dependency from core, it would require to import the same dependencies as core does. If we eventually upgrade some core/ dependency, we would have to upgrade scripts as well. Why having such headache if we can center all dependencies into a single source of truth in the root folder? Again, I see no benefits in splitting multiple package.json inside the same project, I even see problems on doing that.

matuella commented 2 years ago

I don't remember exactly when you changed your mind about it, but I remember that you said to me that we faced so many issues trying to move the functions/ folder outside that you gave up.

That was before deciding to go with /src.

Honestly, in my personal opinion both structures doesn't bring any kind of benefit, they're just different styles

I get that, after all, there are some things that are simply similar and give no clear benefit when comparing two different approaches, at least not now. The only "benefit" that I see here is that everything that is related to the server (the CODE) is inside /src, and we will keep the root of the project completely independent from code, other than toolkit-related files. I don't see why we should change that.

Imagine if we had a package.json under scripts/ folder and another one under core/ foder. If scripts uses any dependency from core, it would require to import the same dependencies as core does

But the problem is about bundling. If we'd have dependencies specific in scripts and in functions, why would scripts need this same dependency? If we had other "bundle-able" code, such as another application that should be hosted somewhere, would it make sense to bundle it with firebase as well?

ggirotto commented 2 years ago

But the problem is about bundling. If we'd have dependencies specific in scripts and in functions, why would scripts need this same dependency? If we had other "bundle-able" code, such as another application that should be hosted somewhere, would it make sense to bundle it with firebase as well?

See tests as an example. scripts will in most of the cases use use-cases, so the dependencies that scripts have functions will also have, but if you take a look at tests, it may be the case that we need to install the same dependency that we have in src/core in tests as well. See this example. There we need to install joi dependency in the root package.json so the tests under test/ can use it to validate schemas. This is an example of the problem that I'm saying we'll have: If joi versions mismatch between src/core/package.json and the root package.json, the tests will break.

About the bundling, I'm not sure what is your concern. All the dependencies that relate to test/ or script/ will certainly be under devDependency, which are not bundled. The only dependencies that we'll need to specify in dependencies section are those that are certainly used under src/core. sinon and mocha are examples that are used in tests only and will not be bundled when uploading src/core

ggirotto commented 2 years ago

@matuella Updated based on the discussion we had in Discord