search

m4ss1m0g / mediatr-ts

Mediator patter for Typescript, inspired to MediatR csharp library
MIT License
46 stars 9 forks source link

feat: improve strong-typing and conventions #26

Closed ffMathy closed 1 week ago

ffMathy commented 2 weeks ago

I am back to contributing to this library after a long break. The times I've wanted to use it have been many, but every time I feel like it's a bit of a burden when the project grows due to a couple of things. This PR fixes some of those.

Updating naming to better match TypeScript naming conventions

This is a port of a C# library. Although it needs to be familiar, it should follow TypeScript naming conventions. For instance, interfaces are not prefixed with I. This is also discouraged by the TypeScript team, including the creator of TypeScript and C#, Anders Hejlsbjerg.

Here's more reasons: https://stackoverflow.com/questions/31876947/confused-about-the-interface-and-class-coding-guidelines-for-typescript

Full list of renames:

Bumping the version to 2.0.0

This is because this PR contains breaking changes.

Removing deprecated properties

I have removed all deprecated properties, which is okay, since the major version number has also been bumped.

Changing Request and Notification to be classes instead of interfaces

This one might seem controversial, but let me explain.

In the current version of mediatr-ts, type inference doesn't work properly.

class Request implements IRequest<string> {
    name: string;
}

@requestHandler(Request)
class HandlerTest implements IRequestHandler<Request, string> {
    async handle(value: Request): Promise<string> {
        return `Value passed ${value.name}`;
    }
}

const mediator = new Mediator();

const r = new Request();
r.name = "Foo";

// Here, I specify `<string>` to make the return type a string. This shouldn't be necessary, as Request already has the return type inside.
const result1 = await mediator.send<string>(r);

// But here, if I try to remove the `<string>` part, `result2` is just of type `unknown`.
const result2 = await mediator.send(r);

This is caused by empty interfaces and types in TypeScript combined with Duck Typing which makes it impossible to do. Classes don't have this limitation.

So now instead of implements you have to do extends, but I think that's fine. IRequest has also been renamed to RequestBase. I couldn't name it Request, as that conflicts with an inbuilt type in EcmaScript already.

Here is the same example with the new code: 

class Request extends RequestBase<string> {
    name: string;
}

@requestHandler(Request)
class HandlerTest implements RequestHandler<Request, string> {
    async handle(value: Request): Promise<string> {
        return `Value passed ${value.name}`;
    }
}

const mediator = new Mediator();

const r = new Request();
r.name = "Foo";

// Here, `result` is of type `string` as would be expected.
const result = await mediator.send(r);

It also does type checking of the RequestHandler type, and makes sure it matches the type given in the request.

We might be able to further improve the type inference and make it even better than the C# version since TypeScript's type system is more advanced, but that's for a later PR.

Getting rid of mediatorSettings

To me, it complicates the setup a bit. Instead, I have changed Dispatcher and Resolver to use the Singleton pattern. However, a custom resolver and dispatcher instance can still be set inside the Mediator constructor.

Updated documentation

The documentation should now reflect the new changes. I also fixed a few issues that weren't actually working.

Removed examples

I removed the examples directory. I feel like the examples in the README.md are sufficient.

Thoughts?

Let me know what you think. I am willing to discuss these changes and I am also very flexible in terms of changing some of it.

ffMathy commented 2 weeks ago

cc @m4ss1m0g! :heart:

ffMathy commented 2 weeks ago

@m4ss1m0g just a heads-up since I really need this.

In about a week or two, I will fork this repo and publish my own version of mediatr-ts to NPM if I don't hear from you ☺️

m4ss1m0g commented 2 weeks ago

@ffMathy Thanks for the PR, It's also been a long time since I actively developed the project, your PR is exciting and also solve the #25 issue. I check your code and if ok I'll merge it.

ffMathy commented 2 weeks ago

I agree with most of your points.

However, as for the naming, like you're suggesting, I'll leave it without "I" due to the link I sent.

What issues do you see with not having "I" in front of the name?

m4ss1m0g commented 2 weeks ago

What issues do you see with not having "I" in front of the name?

Without the I in front of the name, it creates confusion because you don't know if is an interface or class, when you know the code is easy, but when you write the code for the first time you must search if you coding against an interface or class.

On the PR you have this code export default class Resolver implements ResolverInterface

You have a problem too ;-) to identify the interface and the class.

The typescript book suggests avoiding the I and changing the name of the implementation, the code above must be

export default class ResolverImpl implements Resolver

The main reason to avoid the I is that the typescript interfaces are different than other languages, but for me are always interfaces, and adding a I in front of the name clarifies instantly what it is.

From the first comment on the StackOverflow post

To clarify, this is the documentation about the style of the code for TypeScript, and not a style guideline for how to implement your project. If using the I prefix makes sense to you and your team, USE IT. If not, maybe the Java style of SomeThing (interface) with SomeThingImpl (implementation) then by all means use that

Conclusion:

In typescript its suggest to remove the I and Add the Impl In CSharp its suggest to add the I and remove the Impl

Each language has its suggested naming convention, and each thinks it is better than the others.

But all are suggestions, I think we are obsessed with the style and do not care about the code, this is my personal opinion of 40 years of dev.

ffMathy commented 2 weeks ago

Without the I in front of the name, it creates confusion because you don't know if is an interface or class, when you know the code is easy, but when you write the code for the first time you must search if you coding against an interface or class.

I think the point is to not use either. From the StackOverflow post:

Let's assume you get some black-box. You get some type reference that allows you to interact with that box. You should not care if it is an interface or a class. You just use its interface part. Demanding to know what is it (interface, specific implementation or abstract class) is a violation of encapsulation.

In other words, either using Impl or I is bad, because implicitly knowing what it is, breaks encapsulation. This is actually also argued among C# and Java devs - it's just a bad habbit that is very hard to get away from. It also makes you more likely to break the Interface Seggregation Principle.

You have a problem too ;-) to identify the interface and the class.

Yes, but within the library. I do not actually export both the class and interface - I only export one of them. And I think that makes it more okay. I am still not happy with how it looks within the library, and I'll see if I can do something about that, so it's not that I do not agree with your comment here. What I mean though when I talk about the naming, is everything regarding the public API of the package - not its internal workings.

To clarify, this is the documentation about the style of the code for TypeScript, and not a style guideline for how to implement your project. If using the I prefix makes sense to you and your team, USE IT. If not, maybe the Java style of SomeThing (interface) with SomeThingImpl (implementation) then by all means use that.

I would normally fully agree with this and support it. However, I think the key point here is makes sense to your team. This is not enough for a library author. Here, I'd argue catering to what's out there in the wild is the best. I do believe that per-language conventions should win over your own personal conventions for a library. I try to do the same for my libraries too.

I did quite a port of a C# library to TypeScript myself that has become popular, and quite a few other TypeScript projects. The ones I made that have become popular are all following per-language conventions. Not sure if that's a coincidence or not.

When I look at a library from the outside (just as a consumer - seeing what's exported) and I see them using very opinionated conventions, it's not necessarily a huge dealbreaker. But it could be an indicator of the trustworthiness of the library. It could make me second-guess the quality of the rest of the library, and I think that's a shame in this case, because the library is great ❤️

For instance, think of the interface requirement on the IRequest before, that broke type inference. That's actually a huge dealbreaker to me, and is the difference between whether or not to actually adopt it.

m4ss1m0g commented 2 weeks ago

In other words, either using Impl or I is bad, because implicitly knowing what it is, breaks encapsulation. This is actually also argued among C# and Java devs - it's just a bad habbit that is very hard to get away from. It also makes you more likely to break the Interface Segregation Principle.

I strongly disagree (this is PERSONAL opinion). Maybe a dev would know if the black box is an interface or class to think about extending it, instead a user doesn't care about the type but only if works (until it turns to a dev).

When I look at a library from the outside (just as a consumer - seeing what's exported) and I see them using very opinionated conventions, it's not necessarily a huge dealbreaker. But it could be an indicator of the trustworthiness of the library. It could make me second-guess the quality of the rest of the library, and I think that's a shame in this case, because the library is great ❤️

This confirms what I previously said: I think we are obsessed with the style and do not care about the code.

I think each dev has its personal preferences about conventions, when it adopts a new language tends to follow the guidelines to better understand code, until it has sufficient knowledge to create a new personal conventions that can be a "fusion" between the language conventions and its experience.

I develop in many languages, also on X++ where the conventions about naming function parameters must be prefixed with underscore, and the class variable without underscore; since the code interacts with csharp code where the conventions are the opposite what is the solution?

Again each language has its conventions, and each dev has its own. Judge a library because do not follow conventions (for me) it's short-sighted, on the contrary, maybe the dev has experiences with other languages that can improve the library. For me the indicators of bad implementation are essentially two: no comments on code and function with more than 20 lines of code.

[..] I do believe that per-language conventions should win over your own personal conventions for a library. I try to do the same for my libraries too.

To be clear: I developed this project for my usage, and then I turned it into a public npm; obviously, I wrote it with my personal conventions (when I wrote it I already knew about the interface naming convention), since now is public naming can follow the conventions.

In this particular case, for me the I better explain the scope and feel "comfortable" and I adopt it in my project, but I also recognize that violate the typescript convention about the interfaces and I will not oppose its implementation.

ffMathy commented 2 weeks ago

Maybe a dev would know if the black box is an interface or class to think about extending it, instead a user doesn't care about the type but only if works (until it turns to a dev).

What I mean is that this goes against a lot of literature and a lot of articles. But of course you can have your own opinion, for sure!

I develop in many languages, also on X++ where the conventions about naming function parameters must be prefixed with underscore, and the class variable without underscore; since the code interacts with csharp code where the conventions are the opposite what is the solution?

This is a good argument. And this is also where we enter the land of opinion. I guess it's partly about where your users primarily are - I agree with that. But people who come from MediatR in C# and land in this library will also know about TypeScript conventions. People that come from pure TypeScript will not know about C# conventions.

Again each language has its conventions, and each dev has its own. Judge a library because do not follow conventions (for me) it's short-sighted, on the contrary, maybe the dev has experiences with other languages that can improve the library.

I am not saying it's not short-sighted at all, and with that I agree. I am just saying that if we want this project to succeed and people have a lot of libraries to pick from, it makes sense to give them the "trust" already when they look at the usage examples at first glance. I see this as a funnel:

  1. People discover the repo.
  2. They glance through the usage examples.
  3. If it is feels trustworthy, maybe they'll consider using it for a smaller project. If not, they move on.
  4. If it works for the smaller area of code or project, they will adopt it fully.

So what I am saying is not from the standpoint of "what leads to the most stable system" (which is indeed what matters in the end). I just think that the time between points 3 and 4 are important, in terms of turning this library into a very successful library.

For me the indicators of bad implementation are essentially two: no comments on code and function with more than 20 lines of code.

That's also fine - my indicators are (as you say) different, so your point here is also valid. What I mean to say is: why not try to elliminate as many of the most common things that people consider is bad code, to increase adoption?

I think we are obsessed with the style and do not care about the code.

Don't get me wrong, if this is meant to just be a personal project for you, that's totally fine. Then it also makes sense that you would care mostly about how you perceive it, and not others. You are certainly (of course) free to do so.

I wrote these comments to assist with some information I think could be of use to you - based on a lot of experience with other libraries in the past. This is all because I truly believe this could matter, and that this library could become something bigger - larger than just your own usage. That would be awesome in my opinion.

But if you primarily want it for your own, that's also OK - I can always just fork it and publish the fork.

m4ss1m0g commented 2 weeks ago

I wrote these comments to assist with some information I think could be of use to you

I certainly draw a lesson from it, I had never thought that strict adherence to conventions could lead to the success of a public package/library. I will have something to ponder in the next public project.

But if you primarily want it for your own, that's also OK - I can always just fork it and publish the fork.

No absolutely, I already answered in my last sentence where I said that you can change what you want to adhere to TS conventions.

m4ss1m0g commented 2 weeks ago

Since you have rewritten most of the code, what do you think about these two issues?

brunovinicius commented 2 weeks ago

Hello everyone. I am a little late to the party but I wanted to call attention to the issue I raised on #27. Since we are making big changes (most of which I am in full support of) I would like defend that we should not expose the Resolver at all.

You see, its somewhat pointless. Its important to let users create the object but its not important and counter productive (see my issue #27) to ask him to do so. What a user wants to provide to extend the functionality and integrate with DI is a factory for the handlers, not the full resolver. I mean, the implementation we got for resolvers is very good and it doesn't make much sense to ask the user to provide one anew.

As I explained on #27, since we are relying on attributes to define request handlers it makes it mandatory that I set configuration for mediatr before I import the handlers, otherwise the calls to add will go to void. This creates a scenario where changin order of imports might break the application completely which is very counter intuitive to any developer.

What I suggest compared to what we have in this PR:

export abstract class Factory {
   abstract getInstance<T>(commandName: string, handler: Function): T;
}
ffMathy commented 1 week ago

At first glance, both of your replies are really good. Will take a look within the coming weeks and do my best to accommodate everything.

Then I will also reply to both properly.

m4ss1m0g commented 1 week ago

You see, its somewhat pointless. Its important to let users create the object but its not important and counter productive (see my issue #27) to ask him to do so. What a user wants to provide to extend the functionality and integrate with DI is a factory for the handlers, not the full resolver. I mean, the implementation we got for resolvers is very good and it doesn't make much sense to ask the user to provide one anew.

In this case, it is not possible to modify the Resolver. When I created the package I thought about the Inversify library, and the freedom to choose the best library for DI for you.

As I explained on #27, since we are relying on attributes to define request handlers it makes it mandatory that I set configuration for mediatr before I import the handlers, otherwise the calls to add will go to void. This creates a scenario where changin order of imports might break the application completely which is very counter intuitive to any developer.

I agree with you; in some scenarios orders on import matters.

What I suggest compared to what we have in this PR:

  • Change the Resolver so its no longer exported
  • Update Resolver so that it returns the type of the handler, refered to as fx on other methods
  • Reinstate the resolver as a private depency of Mediator; always there cannot be changed
  • Instead of having optional Dispatcher and Resolver as parameters on the constructor, get a optional Factory that is invoked whe we need to instantiate the Handler
  • Factory only has one method getIntance that takes the command name (string) and handler class (Function or maybe typeof T) as parameters and returns a T
  • By default, Mediator get a simple factory implementation that only creates a new instance through new
export abstract class Factory {
   abstract getInstance<T>(commandName: string, handler: Function): T;
}

If I understand correctly, with your solution, there is only the internal Resolver, and to change something about the resolver you must use the Factory Impl to pass to the constructor.

Maybe I don't understand your point of view but why yout don't create your personal Resolver and passing it to the Mediator constructor and customize the resolve and add methods?

brunovinicius commented 1 week ago

@ffMathy I am open to contribute, just say the word.

@m4ss1m0g

In this case, it is not possible to modify the Resolver. When I created the package I thought about the Inversify library, and the freedom to choose the best library for DI for you.

Absolutely. And it was great! Without it I would not have managed to adapt it to Angular DI.

Maybe I don't understand your point of view but why yout don't create your personal Resolver and passing it to the Mediator constructor and customize the resolve and add methods?

I actually implemented a custom resolver on my project, I just dealt with the import order issue by making sure the order is proper. I can't get over the fact that if I add a import sort plugin on my prettier config, the app will break. This is the issue I am trying to address.

If I understand correctly, with your solution, there is only the internal Resolver, and to change something about the resolver you must use the Factory Impl to pass to the constructor.

You understood correctly. If we go with my implementation you are losing effectively the ability to rely on Mediatr to configure the DI for you. That's a considerable downgrade.

We can address this by making sure we call add on the custom resolver when we get access to it via constructor. We can make sure that this is the case by adding extension points to the Resolver singleton instead of depending on the user to completely replace it.

I think this makes sure we support both cases. With getInstance passing the function it also allows for users to optionally rely on add or only use implement the getInstance like in my case with Angular. Here's some example code, please don't mind code conventions I used. Also, I'm not crazy about the bridge naming, feel free to suggest better alternatives :)

export interface DIBridge {
   add(commandName: string, handler: Function): void;
   getInstance(commandName: string, handler: Function): T;
}

class SimpleDIBridge implements DIBridge {
   add(commandName: string, handler: Function): void {
   }

   getInstance(commandName: string, handler: Function): T {
      return new handler() as T;
   }
}

class Resolver {
   _bridge = new SimpleDIBridge();

   set bridge(bridge: DIBridge) {
      // for every command to handler mapping call add on the new bridge

      this._bridge = bridge.
   }

   resolve<T>(commandName: string): T {
      // existing code...

      return this._bridge.getInstance(name, handler);
   }

   add<T>(commandName: string, handler: Function): void {
      // existing code to make sure we never let any mapping unattended...

     this._bridge.add(commandName, handler);
   }
}

One of the pros of this solution is that we keep the resolver really hidden and encapsulated. It also can now be a true singleton.

brunovinicius commented 1 week ago

one more thing: first of all congrats to you both @ffMathy and @m4ss1m0g on the polite discussion over the interface/implementation naming conventions. I can't resist to chime in with my opinion, but I'll keep it short:

  1. @ffMathy made a great argument that I agree wholeheartedly: this lib is gold. People around the world need this. They might think they don't but thats bc they are dumb as fuck :D

  2. I think the worst we can do is to come up with a different naming convention to both C# and TS.

In the interest encouraging adoption and inviting more TS programmers to contribute, I think we must bite the bullet and go with the TS convention. Between us 3, we are already sold. We should make the decision based on what's best so more people can join us :)

ffMathy commented 1 week ago

@brunovinicius thanks for the offer of contributing, but I think it'll be a bit complicated if we both do a PR.

But maybe after my attempt sometime next week, we can align.

I'll do my best to accommodate your issue and also the issues with the PR.

m4ss1m0g commented 1 week ago

@brunovinicius

Preface

Most probably I don't fully understand your problem (maybe the language barrier, I'm Italian...)

From my personal experiences, I hate libraries/code that keep most of the code hidden without a reason, often I found code that was not usable because most of it was private or sealed (I know in JS there is no this kind of problem it is all a convention).

Code

I saw the code about Bridge implementation, you can achieve the same exact code right now by implementing a custom resolver, the only drawback is that you must import it before any others.

For the orders of import with the new implementations of @ffMathy if you use the default Resolver you don't need other stuff, because the default Resolver is imported with the requestHandler attribute.

/* eslint-disable @typescript-eslint/ban-types */
import type { RequestClass } from "@/models/request.js";
import Resolver from "@/models/resolver.js";

/**
 * Decorate the requestHandler with this attribute
 * 
 * @param value The request type
 */
const requestHandler = <T>(value: RequestClass<T>) => {
    return (target: Function): void => {
        const name = (value as Function).prototype.constructor.name;
        Resolver.instance.add(name, target);
    };
};

export default requestHandler;

The problem came back if you want a custom resolver, in this case, you have to instantiate it before any calls to requestHandler, and in this case order matters.

We are waiting for @ffMathy's implementations, maybe it solves your problem or can suggest a solution, but I prefer not to change the interface with a Bridge, unless it has an advantage.

Conclusion

I came from C# word, when I developed this package I tried to ad-here (mimic) the same behavior of IMediator library; on a .NET project that uses this lib, you have to configure a DI container and then use the mediator.

From my personal point of view, the solution with the bridge is a little bit complicated rather than a simple interface to implement.

Sorry if I missed something, but it's been a long time since I developed this package, anyway thanks for your time to contribute to this package.

ffMathy commented 1 week ago

Guys, I took a step back and re-looked at this. Originally I went for implementing @m4ss1m0g's feedback, but then I got another idea that could actually fix all issues.

Here is the commit where I did a new attempt: https://github.com/m4ss1m0g/mediatr-ts/pull/26/commits/27d48fa022c89fc69fc0435111ec2c9ac0e196c6

Notable changes:

The above fixes the following concerns:

  1. The order of imports for Mediator instances no longer matters. This should fix #27!
  2. You can have multiple instances if you want, and each instance could have its own settings.
  3. We no longer have confusion of classes versus interfaces, as the superfluous code has just been deleted. Since TypeScript is duck-typed, it's OK to just export a class directly.
  4. Our test cleanup is a lot simpler.
  5. The Singleton issue is gone. Only the private and internal type map is a singleton, which makes sense, because that mapping never changes.

If you like the above, I still need to work on the following:

  1. I need to update the documentation.
  2. Currently it's no longer possible to set the order of notification handlers and pipeline behaviors. I have an easy fix for this in mind.

If you approve of the change so far, I will fix the remaining two issues.

Let me know what you think!

ffMathy commented 1 week ago

Alright, I fixed the last two points too in dd77007.

brunovinicius commented 1 week ago

@ffMathy I tip my hat, great work! This really seems to address #27 and it simplifies everything even further which I am gonna admit is something that I personally love.

The only thing I disagree, is the RequestBase naming. we should go with Request. I understand we don't want to conflict with the JS Request, but its unlikely both these types ever emerge on a single file.

RequestBase feels like something the lib consumer might want to declare a class like this even more now that we are not exporting it as an interface. Not gonna die on this mountain, though.

Otherwise code looks great. Really amazing work.

PS: Edited for clarity

ffMathy commented 1 week ago

Thanks for the feedback!

Actually I went with Request at first. Totally agree! The problem is that Request is an inbuilt interface in TypeScript in web environments even without importing anything (it's a global). I think that can create a lot of confusion and issues, and I think it makes VS Code not recommend importing another type.

In addition, since it's actually technically an abstract class and not an interface (which is already a difference from the original library), I thought maybe the Base suffix would be suitable.

So in other words, I sacrificed a bit of naming there for better developer experience.

Any suggestions for other names? Because I couldn't agree more.

ffMathy commented 1 week ago

Ironically, this could be a real argument for using IRequest as @m4ss1m0g originally suggested too. It just hurts my heart a little bit 😆 ❤️

ffMathy commented 1 week ago

Thoughts @m4ss1m0g @brunovinicius? ❤️

Is the PR good enough to merge?

m4ss1m0g commented 1 week ago

About code

@brunovinicius I have to apologize 😅

Yesterday I took some time to review the project and I remembered that I had created the Resolver in an "automatic" way.

Let me explain: the Mediator library in csharp relies entirely on the IoC Container to resolve both the handler and any injections in the constructor.

I started from this "certainty" and designed the library starting from Resolver, without thinking that in TS, if you use Inversify, the resolution of any dependencies is its responsibility, while the handlers, & Co. are the responsibility of the library.

That said, I believe the Resolver can be safely removed since it is not used and I see no point in creating a custom one.

@ffMathy

Given what was said above I would remove the Resolver, I saw that comments on various files now are wrong or missing (ie. request.ts wrong on header, missing on body), file names are also wrong (irequest.handlers.ts -> can be request.handler.ts).

Below what I would change in the individual files.

mediator.ts

In addition to removing the resolver, I would add some comments inside the send function to better understand how/when the handlers and any behaviors are called.

mapping.ts

There are no comments at all 😃. I would divide the file into 3 parts + 1 (and comments 😃):

Inside ordered.mappings.ts OrderedMappings and the function byOrder

Inside index.ts I would put the export of typeMappings

About naming Request

I said earlier that this class is completely lacking comments that can be used by users to understand what the class does, so I think they are more essential than the name.

Below I add the names that I propose.

Having said all this I would like to thank @ffmatty for the great work and patience 🥇

EDIT

@ffMathy I forgot to say that I can make the changes by myself if you don't have time, I'm just interested in what you think about removing the Resolver (@brunovinicius has already said what he thinks)

Another question opened is this #24 issue, do you have encountered it? Do you have seen my solution on beta ? I think can be easily merged with this new structure.

ffMathy commented 1 week ago

That said, I believe the Resolver can be safely removed since it is not used and I see no point in creating a custom one.

I don't understand this, sorry. The Resolver is actually currently being used.

The request handlers can still inject things in their constructor for instance, and should be able to. So to use that with a library of your own choice, you must be able to provide your own resolver, right?

If MediatR wants an instance of a request handler of a certain class, it needs to just know the class, but still ask the resolver for the instance, right?

In addition to removing the resolver, I would add some comments inside the send function to better understand how/when the handlers and any behaviors are called.

I disagree with this unfortunately. After reading books like The Pragmatic Programmer and Clean Code (and also studying how people use comments online), there are numerous reasons comments can be bad.

If a comment explains what something is doing, then typically you can do better by extracting a region of code into a self-explanatory method, or into a named constant, and then you don't need the comment.

However, if a comment explains why something is being done (such as why a particular hack is needed, or why something behaves the way it does), then it's OK.

Comments in general are not the source of truth. The code is. And often they tend to diverge over time. Someone moves comments around and forgets to update them when they change the code. That's why generally I try to instead make the code more readable.

That being said though, the code can still get even more readable, so I will instead try to do that. I will post a new commit and see what you think.

Some of the code is also hard to make clearer, so there I will indeed use a comment (even though I am describing the "what"). In addition, I still agree that public library methods or classes should have comments that explain the what also. That's one of the only exceptions.

Let me know if you want me to send you some literature or blog posts on the matter. I found them quite transformative.

Commit for fix: bea336e

I would divide the file into 3 parts + 1 (and comments 😃)

Yes, this is a fantastic idea. The file was growing too much indeed.

Commit for fix: dff41ce

I said earlier that this class is completely lacking comments that can be used by users to understand what the class does, so I think they are more essential than the name.

Hmmm, the request.ts file already has comments, but I added a few more. Not sure if that's what you meant?

Below I add the names that I propose.

Yeah, still not sure about this actually. I feel like both Data, Details and Info then make it diverge more from the namings of Mediator. Don't get me wrong, RequestBase still diverges from Request, but at least Base is a common suffix that is put on abstract base classes.

And I still think RequestBase is better than IRequest actually (because now it is actually a class instead of an interface). But only a little. I am still not fully satisfied. Request would have been so great.

I mean, technically it's only going to be a problem if people in their tsconfig.json have a lib of EcmaScript instead of Node, but I believe a lot of people have that.

I would love to hear what @brunovinicius has to say here too :heart:

Having said all this I would like to thank @ffmatty for the great work and patience 🥇

Thank you so much for this! I love contributing back to random projects, and for me I also see it as training and a learning experience (keeping the tools in my toolbox "sharp"). Thank you for responding so nicely and constructively (and fast). We have a good discussion going here - I love it.

I'd be honored to become an official contributor (write access to the repo) if you need further assistance or have too much on your hands, but of course that's totally optional.

brunovinicius commented 1 week ago

from my point of view:

  1. Most of the changes proposed by @m4ss1m0g are very reasonable.
  2. I also agree with @ffMathy on comments overall. Good code tends to be self explanatory and I feel it is. Maybe we want to expand on the documentation of the dependencies or the class itself.
  3. On the Request class naming, I actually like RequestData very much, as well as RequestDetails to a lesser degree.
  4. Finally, the new version of the Resolver as implemented currently allows extension of Mediator to plug to various DIs containers and fixes #27. I think it should stay as is.

Furthermore, this PR is already too big. Request base is not even that bad, maybe we want to close it off and discuss more on the Request class naming issue without publishing the major version right away. I would suggest deploying a alpha version so we can feel it out on our codebases and then provide more feedback if needed.

Again, cheers to all of you guys. This thing became even more sleek than it already was. An outstanding job.

ffMathy commented 1 week ago

@brunovinicius great input - do you feel like the two commits I made are fixing the concerns proposed by @m4ss1m0g that you consider reasonable?

Alright, we are 2 people that like RequestData most against 1. Let's go with RequestData.

I'll make that change right away.

ffMathy commented 1 week ago

There we go!

brunovinicius commented 1 week ago

do you feel like the two commits I made are fixing the concerns proposed by @m4ss1m0g that you consider reasonable?

Absolutely yes @ffMathy.

brunovinicius commented 1 week ago

Also, nothing to apologize for @m4ss1m0g. <3

ffMathy commented 1 week ago

That's awesome. Then I believe this can be merged.

At this point, if we need more comments in the code or other minor tweaks or whatever, I believe this can easily be added in a separate PR or just directly on master by @m4ss1m0g (since it's your repo, you can also do whatever you want). I feel like this PR is enough to get a V2 shipped.

m4ss1m0g commented 1 week ago

That said, I believe the Resolver can be safely removed since it is not used and I see no point in creating a custom one.

I don't understand this, sorry. The Resolver is actually currently being used.

Actually, if you do not pass settings an (empty) InstantiationResolver is created, the add is empty and then you call resolve for return new type();

The question is: do you think there are any reasons for a custom resolver? At this point you can leave the Resolver as is, it provides another level of customization but I don't see many reasons to use it.

I disagree with this, unfortunately. After reading books like The Pragmatic Programmer and Clean Code (and also studying how people use comments online), there are numerous reasons comments can be bad.

I read Refactoring (improving the design of existing code) and I remember Martin says that comments are not evil, it says that most of the time are used "as a deodorant".

I have the opposite view, my direct experiences say the other (an entire project without a comment because "the code is already explicative" except for the fact that nobody knows what doing some (BIG) pieces of code)

When I first read your behavior code I had a hard time understanding what it did exactly (I don't develop a lot of code in TS), and even now if I read what the send method does, I have to sit there and spin the code in my head.

We are not all the same, maybe I'm not as skilled as you on TS but I would like to contribute to the project, then I open the code and I don't understand it all and I don't find any comments and I give up.

I won't add anything else, we have different visions and I don't want to convince you of my idea.

At this point, if we need more comments in the code or other minor tweaks or whatever, I believe this can easily be added in a separate PR or just directly on master by @m4ss1m0g (since it's your repo, you can also do whatever you want). I feel like this PR is enough to get a V2 shipped.

Yes I'll try to add comments if needed.

When you is ready I merge.

EDIT

Sorry, I read you have done enough and the code is ready, I'll merge.

m4ss1m0g commented 1 week ago

Fix also the #27

m4ss1m0g commented 1 week ago

@ffMathy Sorry for bothering you, but I found the [marker] property is not used, I think can be safely removed, what do you think?

const marker = Symbol();

/**
 *  The request interface
 *
 * @export
 * @interface RequestData
 * @template T
 */
// eslint-disable-next-line @typescript-eslint/no-empty-interface,  @typescript-eslint/no-unused-vars
export default class RequestData<T = void> {
    // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
    [marker]: T = null!;
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type RequestDataClass<T> = new (...args: any[]) => RequestData<T>;

EDIT

Uhmm I think is used for some kind of placeholder for generic T

ffMathy commented 1 week ago

It has to be there, or else type inference doesn't work.

ffMathy commented 1 week ago

Actually, if you do not pass settings an (empty) InstantiationResolver is created, the add is empty and then you call resolve for return new type();

Yeah, the reason I want the InstantiationResolver is to apply something similar to the Null Object Pattern. By utilizing this pattern, the code that uses the Resolver interface (in this case the Mediator) does not have a high cyclomatic complexity (in other words, there are less if-statements and "paths" the code can take), which leads to less cases needed for testing (either manually or automatically).

I have the opposite view, my direct experiences say the other (an entire project without a comment because "the code is already explicative" except for the fact that nobody knows what doing some (BIG) pieces of code)

Me too. Don't get me wrong, I think Martin is a bit extreme a lot of the times. I just found that often, you can actually do quite a lot for the readability if you follow a few simple tricks.

When I first read your behavior code I had a hard time understanding what it did exactly (I don't develop a lot of code in TS), and even now if I read what the send method does, I have to sit there and spin the code in my head.

Sure thing, I had the same feeling, as I also mentioned. I think it wasn't easy to read. The new version should be better after my latest commit. And like you will see, I also added comments there, so an exception to the rule.

That's because the pipeline behaviors are essentially a Chain of Responsibility Pattern (so a form of "middleware"). And I think that pattern isn't very "intuitive", and is probably hard to make "clean".

I won't add anything else, we have different visions and I don't want to convince you of my idea.

That's totally fine, and I hope you feel like we're discussing this because we're interested in it. I think these subjects are highly interesting, and I think you do share some good perspectives.

It's your project after all - I just wanted to give a reason for why I decided to remove the comments initially, and why I'd love to first try to make it more readable in general :heart:

Great to see this merged! :fire: