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

Discussion #3

Closed
erabti opened this issue Nov 14, 2021 · 2 comments
Closed

Discussion #3

erabti opened this issue Nov 14, 2021 · 2 comments

Comments

@erabti
Copy link

erabti commented Nov 14, 2021

We couldn't find any good json deserialization solution until we found this :) Which solved the biggest problem of generics.

Things I have in mind but of course not sure about them so would love some more insight from you:
1- Merged Generation. (To have one entry file per package):
Having one single file that is generated I think is more tangible for end-user. Friendlier. And you don't to worry about having some entry file for each sub-directory.

2- Making Mapper and few other classes included in the package itself and not having to re-generate for each client-side package.
2-1- We can achieve that by making Mapper as a singleton that we initialize it with the user side custom mappers, this will lead to having to have to call a method like initializeMapper() in main.dart similar to what's injectable doing, the cons are that you have to write one more line, the pros is that you have less code duplicated if you use some kind of modular architecture (given that merged generation is implemented, if not, this will make more sense because Mapper can be duplicated in the same package).
2-2- By having a singleton we can mock Mapper easily and allow better dependency injection which gives users better testability.
2-3- Any changes to Mapper class won't result in need for re-generating your source code (mostly).

3- Make "_mappers" a Set of MapperEntry with generic and BaseMapper instead of a map of type string and a BaseMapper.
3-1- This will allow better readability and more user friendly imo.

4- use https://pub.dev/packages/code_builder instead of string snippets for better formatting and development experience.

5- Naming, Mapper is not "is-a" relationship with BaseMapper instead "has-a" which might confuse some. Can't think of better naming though.

6- Maybe divide the generator and the package itself into two separate packages?
So user app size doesn't get affected by builders.

7- Maybe use your awesome package https://pub.dev/packages/type_plus instead of doing it ourselves in this package?

8- Some refactor in general and making the internals more type safe and less dynamic as much as possible.

9- Not quite sure of why "decoder" and "encoder" are nullable functions, it will make it harder to deal with them in case we need to use the BaseMapper directly.

I have experimented a bit with some of these ideas to see how well they might work out, this is the initial draft (didn't try to run tests yet and it's still WIP, although I did a little example locally and basic functionalities seem to work, will work more on it once I have more insight from you and time)
https://github.com/erabti/dart_mappable

What do you think? 😊 @schultek

@schultek
Copy link
Owner

schultek commented Nov 14, 2021

Love the initiative. ⚡️

  1. As discussed, I would be open to contributions for this one. But it should still be possible to use it for a single entry file. Also the output file should still be generated relative to the entry file(s) (from what I saw in your fork you instead defined a constant lib/main.mapper.dart output)
  2. That's pretty much already on the todo list, for the exact reasons you mentioned.
    • Singleton: Yes
    • Mandatory initializeMapper() function: I would like to avoid this. One core idea of this package was that it doesn't need any initialization.
  3. Not sure how to get an efficient lookup then. In your fork you iterate over all entries, which I would like to avoid.
  4. Yes, definitely on the todo list
  5. I'm bugged by the same thing. Mapper, BaseMapper, Mappable, etc. are all so similar but I couldn't come up with something better.
  6. I was always wondering about this one. Doesn't flutter use tree-shaking that would ignore the builders anyways? That doesn't apply to pure dart though. Do you have some insights for this?
  7. Also on the todo list, but quite tricky, so I was putting it off 😅
  8. Gladly
  9. Because you can deactivate generation of decoding/encoding methods: https://pub.dev/packages/dart_mappable#generation-methods
    • [Edit] After thinking about this, these could also instead throw UnimplementedExceptions or some similar error by default. Maybe this makes it even clearer for the user if he wrongly tries to call them.

Generally there is quite some refactoring already on the todo list, especially when looking at a possible stable release. As you know, this was my first package doing code-generation stuff, so there are a lot of not-so-best-practices that need cleaning up. I'm really happy about the feedback and any further help in developing.

@schultek
Copy link
Owner

Fyi I published a prerelease version 1.0.0-dev.0 that includes most of the above points.

I will now close this issue because of inactivity. When you want to discuss a specific point again feel free to open a new issue on it.

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