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

We really need builders for these model instances #154

Closed
Tracked by #216
dgaedcke opened this issue Mar 14, 2023 · 25 comments
Closed
Tracked by #216

We really need builders for these model instances #154

dgaedcke opened this issue Mar 14, 2023 · 25 comments
Labels
good first issue Good for newcomers

Comments

@dgaedcke
Copy link

dgaedcke commented Mar 14, 2023

I often use the Prisma generated code on the CLIENT SIDE (talking directly to the DB) for prototyping and client demo's, to help them flush out their specs and requirements. For server code, it makes total sense that all the Model classes are immutable and must be constructed all-at-once (with all fields) from a client API request.

But on the client side, data is collected in pieces, across many screens, and I hate having to manually clone all the Prisma data structures so I have data-models on which to store user-input from the UI!!

So I'm having serious headaches using existing models to accumulate data across multiple forms. Due to all attributes being "final", we can't construct any of these generated models until ALL FIELDS (data properties) are available, and for complex flows (eg large tables), that takes many screens.
This means I'm having to pass loosely typed JSON maps (user form-input) along from screen to screen, or else I need to manually clone your generated table-models with NON-FINAL fields, so I have strongly typed entities to pass forward in my screen flow.

Can you either:

  1. teach me a better way to do this; or
  2. use code generation to create model-entity "builders" so I we accumulate model values over time, and then use the builder to produce the final immutable model-instance when all data is available?

For example, built-value generates builder classes automatically for immutable models:
https://youtu.be/hNbOSSgpneI

Other than that, I'm really loving this tooling ... it's very well done!!

@medz
Copy link
Owner

medz commented Mar 14, 2023

@dgaedcke Thank you for your endorsement of Prisma ORM for Dart, for the two schemes you belong to:

teach me a better way to do this;

Without changing the existing generator, all Inputs classes support fromJson for construction, you can use it. PS: As far as I know @jackstefansky @nikosportolos is experienced with this.

use code generation to create model-entity "builders" so I we accumulate model values over time, and then use the builder to produce the final immutable model-instance when all data is available?

Regarding adding code to the generator to support this situation, we usually add the option copyWith, there are two ways:

  1. Submit a PR to this repository, the generator is located in bin/src/generator.dart, and add a copyWith method to all model classes.
  2. You can rely on our newly created tool https://github.com/odroe/prisma-generator-helper Using this tool, you can get Prisma schema DMMF. You can mimic the Prisma Client Dart generator to generate what you need, such as generating copyWith extensions for all input classes of the orm package.

@medz
Copy link
Owner

medz commented Mar 14, 2023

Perhaps, we have another option, adding configuration to the generator. Tells the generator to generate mutable accumulated member properties or final member properties. And this is the solution with the least modification, because we only need to change the inputs generator and refuse to add the final keyword. Of course, you also need to remove the const keyword from the class constructor.

@medz medz added enhancement good first issue Good for newcomers labels Mar 14, 2023
@medz
Copy link
Owner

medz commented Mar 14, 2023

@medz
Copy link
Owner

medz commented Mar 14, 2023

@dgaedcke I create a separate branch for you, implementing mutable class member properties.

You depend on it via git, make sure it's what you need. If yes, we release it as a new effect in the next version.

@medz medz linked a pull request Mar 14, 2023 that will close this issue
@medz
Copy link
Owner

medz commented Mar 14, 2023

@dgaedcke I created a scheme in the https://github.com/odroe/prisma-dart/tree/154-mutable-class-member-properties branch, you can confirm whether it is what you need by relying on it through pub's git .

You can see #155 PR for details.

Looking forward to your feedback.

@dgaedcke
Copy link
Author

dgaedcke commented Mar 14, 2023

Thanks so much for the speedy response!!
"copyWith" is not a good solution because it's not possible to create the first instance without having ALL of the fields filled out ... and that's the very problem we're trying to avoid. The developer should not have to use fake values in the constructor, just to get a new instance (whether or not it is mutable).

I like your solution of generating the models with or without "final" as a first step. But that really should be a temporary solution because the codebase really should have immutable values.

Here's a good compromises for first step:
npx prisma generate --model-classes-non-final
npx prisma generate --create-classes-non-final
npx prisma generate --update-classes-non-final

so by running:
npx prisma generate --create-classes-non-final --update-classes-non-final

my model classes will be be immutable, but my data-collector (create & update) classes are mutable. What do you think of that approach??

It would be even better, if the Create and Update classes has a fallible method (tryBuildModel) to convert into a model ... without sending to the DB ... something like this:

var addressCreateInput = AddressCreateInput()..field1=''..field2='';
try:
   Address addrInstance = addressCreateInput.tryBuildModel();
catch: ...
class AddressCreateInput implements _i1.JsonSerializable {
  const AddressCreateInput({  });

  factory AddressCreateInput.fromJson(Map<String, dynamic> json) =>
      _$AddressCreateInputFromJson(json);
  ....
  final AccountCreateNestedOneWithoutAddressesInput account;
  @override
  Map<String, dynamic> toJson() => _$AddressCreateInputToJson(this);
}```

@medz
Copy link
Owner

medz commented Mar 15, 2023

@dgaedcke I should understand what you said, what you need is a scheme like built_value.

But now there is an incompatibility problem, built_value and json_serializable are not compatible, they are all fully constructed *.g.dart.

If our generator adds *Builder to the input class, it will make the Prisma client extremely large (just like using freezed at the beginning, the client is huge and the compilation speed is very slow)

Can we make a choice and let developers choose built_value or json_serializable. But I don't recommend it, because it will cause the Prisma Client structure generated by each developer to be different.

@medz
Copy link
Owner

medz commented Mar 15, 2023

Maybe we need to create tools like zod and prisma-zod-generator, or something like json-schema. But it's hard in Dart. These tools are based on the type safety of TypeScript's paradigm + calculation type. In Dart, we can only verify JSON, but we cannot do JSON-built security reminders and editors can only remind and verify.

Because Dart does not support type calculations. Generic support is also poor.

@dgaedcke
Copy link
Author

dgaedcke commented Mar 15, 2023

It seems the "client different for each developer" problem can be solved by convention -- each development team can select their preferred approach. Also, I'm confused by your comment that Generic support is poor in Dart ... I've not found that to be true but it seems you are familiar with some design-pattern that I've never had to face.

I have a 3rd compromise solution. How about:
npx prisma generate --create-mutable-shadow-models

This would create another model structure as follows:

class AddressShadow {
  // mutable -- properties / attributes

   // methods below would be fallible -- they could fail if not enough values are set
   AddressCreateInput buildCreateInstance() {}
   AddressUpdateInput buildUpdateInstance() {}
   Address buildModelInstance() {}
   bool confirmRequiredFieldsAreSet() {}
}

These shadow classes would not add a lot of size (only one class per DB table) but would give all of the flexibility I'm requesting!!

Thanks so much for your engagement and support with this issue :-)

@medz
Copy link
Owner

medz commented Mar 16, 2023

I'm confused by your comment that Generic support is poor in Dart ... I've not found that to be true but it seems you are familiar with some design-pattern that I've never had to face.

for example:

  • Union-Types
  • Computable generics, T extends num -> T of int ? int : double

Regarding the discussion of this issue, I think we need to collect feedback and solutions from more developers. Because this change is so big, it is almost a breaking change to the generator.

I will pinned this issue and send a tweet to make sure more developers see it.

https://twitter.com/odroeinc/status/1636222741295816704?s=20

@medz medz pinned this issue Mar 16, 2023
@dgaedcke
Copy link
Author

Sounds great!! Thank you --
I hope we get some feedback or solutions that will make it easy to construct instances progressively (not all at once). If we have to do it all at once, and we want benefit of strong types, then we need to create duplicates of all the models ... the only other solution is to carry all data around in Map<String, dynamic>{} and we lose benefit of type checking and enforcement

@medz
Copy link
Owner

medz commented Mar 16, 2023

Also, I think it can be implemented along with #166. Because the implementation together greatly improves the ease of use.

In particular we use web frameworks like spry/shelf to implement the server side and then share the model with the client.

@dgaedcke
Copy link
Author

dgaedcke commented Mar 16, 2023

Yes!! Thank you.
Being able to auto-generate "Shadow" classes that can be used on the client side to collect user-input would REALLY boost my productivity!! It would also make field-names line up perfectly in the JSON between client and server!!

@dgaedcke
Copy link
Author

dgaedcke commented Mar 16, 2023

BTW ... if we need Union-Types in Dart, is there some reason that combining code-generation, generics and Built-Value (or Freezed) won't let you express anything you can express in TypeScript??
https://pub.dev/packages/freezed

@nikosportolos
Copy link
Contributor

@medz Might also worth looking at https://pub.dev/packages/data_class_plugin.
It supports all common data class methods (hash/equals, copyWith, from/toJson), unions, etc. and it's not dependent on build_runner.

@medz
Copy link
Owner

medz commented Mar 17, 2023

BTW ... if we need Union-Types in Dart, is there some reason that combining code-generation, generics and Built-Value (or Freezed) won't let you express anything you can express in TypeScript?? https://pub.dev/packages/freezed

@dgaedcke Whether the Prisma engine is the GraphQL protocol or the experimental JSON protocol, a short type always has multiple structures.

For example, UserWhereInput.age, it may be an int type of 18, but it may also be an Object of { gt: 18 }, or { 'gt': { $ref: 'age2' } }.

So the current Prisma ORM for Dart only selects the longest input type among them, because there are no Union-Types.

generics and Built-Value (or Freezed)

Developers who have experienced the 2.6 version seem to be aware that freezed has its own performance problems. There is also a bug about the wrong code implementation of toString in freezed. I don't know the author's reason, when I submitted the Issue, he asked me to fix it. When I really fixed the problem for him with a PR, wrote the test code, and met all the requirements of his Review, he ignored it. I know he has the right to ignore my PR (because it is his project), but when I see many PRs in the future being processed quickly, I feel that I have been targeted and discriminated against (open source has no borders, but writing code Man! I'm a huge anti-discrimination person, and now I express my displeasure by complaining here and closing my PR on freezed and removing freezed from my code)

@medz
Copy link
Owner

medz commented Mar 17, 2023

@medz Might also worth looking at https://pub.dev/packages/data_class_plugin. It supports all common data class methods (hash/equals, copyWith, from/toJson), unions, etc. and it's not dependent on build_runner.

I'm keeping an eye on it, but I haven't tested it yet and don't know if it can generate code via cli.

@dgaedcke
Copy link
Author

I wish we had more feedback from other developers on how best to address this need.
I still vote for simply providing the option to generate mutable "shaddow" classes ... one for each Table / Model class. Seems clean, easy and only adds one class per DB table ... keeping the code-base small!

@medz
Copy link
Owner

medz commented Mar 18, 2023

I wish we had more feedback from other developers on how best to address this need. I still vote for simply providing the option to generate mutable "shaddow" classes ... one for each Table / Model class. Seems clean, easy and only adds one class per DB table ... keeping the code-base small!

@dgaedcke I agree that we should start simple and then address complex needs.

Can you create a PR for this and write a model_example.dart in the project root that demonstrates what you need? Creating a PR helps us start implementing it.

Of course, it would be nice if you could modify the generator. But it doesn't matter, you can start with the model example. I will do related coding work on your PR later.

@medz
Copy link
Owner

medz commented Mar 18, 2023

You can join our discord server https://discord.com/invite/r27AjtUUbV and we can chat more details. issue I feel as an update to the main guide and implementation. Minutiae we can communicate in discord.

@dgaedcke
Copy link
Author

Every time I try to access your server, it asks me to "Claim Account" and every time I do so, it tells me my email is already registered, and won't let me proceed.... not sure what to do. I've been traveling and also had PW probs with DIscord!

@rrousselGit
Copy link

@medz I apologize for the slow review and bad experience. But know that you haven't been discriminated against.

I am working alone on many projects and receive tons of notifications every day. It is not uncommon for one PR/issue to get lost in the sea.
I'm trying to get better at it, but it isn't easy doing all that alone. If you feel like it, I'm sure I can find you a PR that took me 6months to a year to review 🙈

Freezed is a bit in a weird place for me atm too. Because Dart 3.0, a lot of the use-case for Freezed will be invalidated.
And with metaprogramming, Freezed will need a complete rewrite to be ported to macros (since they use neither build_runner nor analyzer).
It'd also fix many issue along the way. For example, the performance issue you made would likely close itself with metaprogramming.
So at the moment, working on Freezd is a low priority for me. Which obviously doesn't help the slow reply issue you faced.

So while I understand your frustration and apologize for it, please do not think that I discriminated against you.
If anything, I would've likely prioritized you if I knew that this was for a separate package trying to integrate with Freezed.

@medz
Copy link
Owner

medz commented Mar 27, 2023

@medz I apologize for the slow review and bad experience. But know that you haven't been discriminated against.

I am working alone on many projects and receive tons of notifications every day. It is not uncommon for one PR/issue to get lost in the sea. I'm trying to get better at it, but it isn't easy doing all that alone. If you feel like it, I'm sure I can find you a PR that took me 6months to a year to review 🙈

Freezed is a bit in a weird place for me atm too. Because Dart 3.0, a lot of the use-case for Freezed will be invalidated. And with metaprogramming, Freezed will need a complete rewrite to be ported to macros (since they use neither build_runner nor analyzer). It'd also fix many issue along the way. For example, the performance issue you made would likely close itself with metaprogramming. So at the moment, working on Freezd is a low priority for me. Which obviously doesn't help the slow reply issue you faced.

So while I understand your frustration and apologize for it, please do not think that I discriminated against you. If anything, I would've likely prioritized you if I knew that this was for a separate package trying to integrate with Freezed.

@rrousselGit Thank you for your reply, and I apologize for the misunderstanding.

Perhaps it was because I received a lot of discrimination in open source work before, which made me very sensitive to this and caused this misunderstanding. Again very sorry.

Also as an indie developer, I can understand how busy you are when maintaining multiple projects. Because I also have to deal with a lot of project codes every day.

However, in my actual test, it is true that both I and the users of this project have encountered Freezed performance problems.

Also, I didn't really remove Freezed from my project because it was nice enough that I could write less boilerplate code, e.g. prisma_dmmf was made using Freezed.

@rrousselGit
Copy link

No worries. It is true that I was slow to reply after-all. And there are indeed some issues with Freezed.

I don't have the time to work on it though. It appears to be not that common (considering the size of the project, I would've expected this issue to come-up a lot more often).
So considering metaprogramming is coming and it'd likely fix the issue; and the fact that I have more important issues to fix at the moment, I doubt I'll work on that issue before porting Freezed to macros.

I'll try to review/merge your toString PR when I have a bit of time though.

@medz medz unpinned this issue May 18, 2023
@medz medz mentioned this issue May 18, 2023
7 tasks
@medz
Copy link
Owner

medz commented Nov 25, 2023

In the new version (v4), I abandoned any JSON serialization tool and in the meantime I developed a tool like zod 👉 https://pub.dev/packages/ovo After the first v4 version is released, I will Start developing this feature. As a follow-up, I will create a separate PR to complete it at that time, which contains:

  1. Validity verification of Input types
  2. Efficient serialization of Types
  3. Action supports direct incoming JSON automatic verification

@medz medz closed this as completed Jan 25, 2024
@medz medz closed this as not planned Won't fix, can't repro, duplicate, stale Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants