-
Notifications
You must be signed in to change notification settings - Fork 6
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
Result.value return type does not match the docs. #194
Comments
Thank YouHi, Animii! I want to thank you for your feedback regarding the I’ve adjusted the typing to explicitly reflect that the value will be Example with the ResolutionWith the changes, here’s an example of how to use the updated implementation: const result = Result.fail("An error occurred");
// Checking if the value is `null` or if the state is a failure
if (result.isNull()) console.log("The result is null or in a failure state:", result.error());
// Using it in a factory method
class UserEntity {
private constructor(public readonly name: string) {}
// Ensure provide null possibly option type
public static create(name: string): Result<UserEntity | null> {
if (!name) return Result.fail("Name is required");
return Result.Ok(new UserEntity(name));
}
}
const userResult = UserEntity.create("");
// now value is possibly null
console.log("User created:", userResult.value()?.name); Beta Version for TestingThis resolution will be published in |
fix: change type create method return null #194
🚨🚨🚨 I understand the adjustments made to the typing @4lessandrodev , but ultimately, it's not great and I think it’s a major regression because it makes things more complex and we end up having to handle more cases. This change introduces a huge breaking change in the existing code using your type-ddd library. When we used Result.fail(), we now have to handle a null value systematically, which in most cases (if not all cases) we don't need to manage. If I take your example, before we simply had to do this:
With your modification, userResult is now UserEntity | null. Any access to a property of the entity or its use in a method (like saving it in a repository) now has to be typed as Another example from my context after integrating your change: In the screenshot, you can see the issue.
And my repository doesn't account for null, and with your change, I’m forced to handle it. We lose a lot of coherence and intuitiveness with this change. I think this needs to be reviewed urgently, and I suggest that, in the meantime, we revert this modification. |
Hi @GaetanCottrez, Thank you for your feedback and for highlighting the challenges caused by the recent changes. To address your concerns, I’ve updated the approach to make the dynamic typing in
This provides flexibility without imposing strict changes that disrupt existing integrations. How It WorksDevelopers can now choose whether to include Scenario 1: Handling
|
New Contract Added:
|
@4lessandrodev Dude! You're very fast and very efficient! A huge thank you once again for your work and your library. When you have time, could you upgrade the version of rich-domain in types-ddd and publish a new version? Also, remember to add the examples described in this issue to the documentation. |
I’ll try to use this new approach when I get the chance @4lessandrodev. However, the try/catch you mention for the infrastructure, isn’t that more for the application layer? Because normally (and in your example applications in other repositories), the domain is implemented by the application, which in turn is implemented by the infrastructure. So the infrastructure doesn't have knowledge of the domain, only the application, and therefore, it should be the one using init() (I’m using type-ddd in a hexagonal architecture). |
Is your feature request related to a problem? Please describe.
The docs in the Result class say, that the
value
method returns null if result is a failure.But the return type is
T
and notT | null
.This could lead to bugs where the null case it not handled.
Describe the solution you'd like
I would like the return type to be
T | null
.Describe alternatives you've considered
Since this would break a lot of code bases, we could at a
valueOrNull
function.Additional context
I don't know if this is intentional, but it took me by surprise.
Awesome project ❤️
The text was updated successfully, but these errors were encountered: