Skip to content
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

Some Architechture Concerns #1752

Open
DScheglov opened this issue Jun 26, 2022 · 3 comments
Open

Some Architechture Concerns #1752

DScheglov opened this issue Jun 26, 2022 · 3 comments

Comments

@DScheglov
Copy link

Hey, @tshemsedinov

Let's look on the sample code.

async ({ countryId }) => {
  const fields = ['cityId', 'name'];
  const where = { countryId };
  const data = await db.select('City', fields, where);
  return { result: 'success', data };
};

It seems the same as an old-school express-handler:

export default async function countryCities({ locals: { db }, body: { countryId } }, res) {
  const fields = ['cityId', 'name'];
  const where = { countryId };
  const data = await db.select('City', fields, where);
  res.json({ result: 'success', data });
}

However, the last one contains the dependency contract but the first one requires code to be read and analyzied to get aware about the dependencies. Impress uses implicit service locator to inject dependencies, so the dependency contracts are always hidden.

Both samples implement business logic inside of the infrastructure level. More then the DB is accessed directly from the request controller,

I guess the good example for that should look like that:

./src/use-cases/get-cities-by-country-id.ts

import type { City } from "./domain";
import type { ICitiesByCountryIdProvider } from "./interfaces";

export const getCitiesByCountryId =
  ({ countryId }: { countryId: number }) =>
  ({ cities }: { cities: ICitiesByCountryIdProvider }): Promise<City[]> =>
      cities.getByCountryId(countryId);

So, what do you think about to make impress more freandly to clean architecture?

@tshemsedinov
Copy link
Member

Import/export (common.js and esm modules) are actually far from clean architecture, because you need to know certain places where other modules located. It's a hardcoded paths (worst case) or dependency locator (best case). But we need dependency injection for low coupling. It's ok to me to have coupling with interfaces (but not from certain files). It's not acceptable to have coupling with not abstract code from certain files. We use v8 sandboxes for DI to build namespaces like domain.moduleName.methodName and we have auto loader for metarhia modules.

Last example is short so putting logic to api handler is ok, bit for larger apps we need domain services located in application/domain. All we wall to achieve with syntax in a first example is to have minimal code, even in if it will be implicit or magic, but it is clear and short.

  • we will never use explicit import/export in any syntax
  • we will never use typascript mixed with javascript code, we use schemas for domain code and autogenerated .d.ts files for metarhia internal modules
  • we will never use class for domain model
  • we will never support ORMs or allow something ORM-friendly

@tshemsedinov
Copy link
Member

@DScheglov
Copy link
Author

DScheglov commented Jun 26, 2022

Import/export (common.js and esm modules) are actually far from clean architecture, because you need to know certain places where other modules located

it is not correct. To import some utilities from certain place doesn't counter the clean architecture. More then it is much better then inject (in any way) absolutely everything.

Clean Architechture doesn't mean absolutely low-coupled code. It means architecture supports incremental development without rewriting the existing code base

It's ok to me to have coupling with interfaces (but not from certain files). It's not acceptable to have coupling with not abstract code from certain files

What is an issue with importing interfaces from some specific file? It is a matter of compiler (transpiler). And actually it is dificult to imagine more abstract code then interface

Last example is short so putting logic to api handler is ok

The Express developers had such considerations too :)
The samples work as patterns.
The violation of principles in any specific project file causes the Broken Window Effect in the whole project

We use v8 sandboxes for DI to build namespaces like domain.moduleName.methodName and we have auto loader for metarhia modules

  • what if we need to run the code on the other engine?
  • what if we need to re-use some code in other nodejs project without impress?
  • domain.moduleName.methodName -- also refers to the specific file. More then it refers in runtime (I hope in some observable future we will have inlining for functions and for imported functions as well, so this approach could block this issue). IMHO, to import from some index file is better then to glue the file structure to module implementation. I'm using nextjs with file-structure-based routing -- it is a pain (because I cannot import whole page tree and have to import each page explicitly).

we will never use explicit import/export in any syntax

  • it makes learning curve steeper but gives any valuable benefit

we will never use typascript mixed with javascript code, we use schemas for domain code and autogenerated .d.ts files for metarhia internal modules

  • it makes runtime performance suffer.
  • the IO data should be verified with schemas but not the domain logic, so such verification should be performed on the infrastructure-layer (controller, repo/dao etc).

we will never use class for domain model

  • we don't use classes at all and so what?

we will never support ORMs or allow something ORM-friendly

  • I support this. Absolutely support. ORM-models tend to leak and to get ubiquitous

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants