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] Fundamental redesign of the library #102

Closed
andrewlock opened this issue Mar 9, 2023 · 18 comments · Fixed by #117
Closed

[Discussion] Fundamental redesign of the library #102

andrewlock opened this issue Mar 9, 2023 · 18 comments · Fixed by #117

Comments

@andrewlock
Copy link
Owner

andrewlock commented Mar 9, 2023

First of all, thanks everyone for your interest in this little source generator, as well as all the proposals for new features to include in the Ids! 🙏

All of the proposals I've seen, and the associated open issues have merit. The trouble is, adding support for all of them would add a huge array of options and flags to the library, making it more and more cumbersome to use, and adding an increasing burden to keep updating the library to add extra options. I don't think that outcome is sustainable or desirable.

Instead, I was considering rebuilding the source generator with a fundamentally different design. Instead of "building in" the definition of an ID into the source generator, consumers of the library would define the "templates" themselves (it would come with default templates matching the existing IDs). The [StronglyTypedId] then selects which template to use. For example, you would decorate your Ids something like this:

using StronglyTypedIds;
[assembly:StronglyTypedIdDefaults("int")] // choose the default template

[StronglyTypedId] // use the default template
public partial struct DefaultId {}

[StronglyTypedId("guid")] // use a Guid template
public partial struct GuidId {}

[StronglyTypedId("guid-efcore")] // use a Guid template with extra converters etc
public partial struct MyEfCoreIdId {}

You would add templates to your project by adding txt files with the name StrongTypedId_<TEMPLATE>.txt to the project. The source generator would read those, to build up a dictionary of templates, e.g.

  • StrongTypedId_Int.txt, "int" template
  • StrongTypedId_Guid.txt, "guid" template
  • StrongTypedId_Guid-efcore.txt, "guid-efcore" template

An example of the template is shown below. The idea is that it's a "valid" csharp class, so it's easy to author them and then just change the extension. You would use TESTID (or similar) as a placeholder for the name of the ID:

readonly partial struct TESTID : System.IComparable<TESTID>, System.IEquatable<TESTID>
{
    public System.Guid Value { get; }

    public TESTID(System.Guid value)
    {
        Value = value;
    }

    public static TESTID New() => new TESTID(System.Guid.NewGuid());
    public static readonly TESTID Empty = new TESTID(System.Guid.Empty);

    public bool Equals(TESTID other) => this.Value.Equals(other.Value);
    public override bool Equals(object obj)
    {
        if (ReferenceEquals(null, obj)) return false;
        return obj is TESTID other && Equals(other);
    }

    public override int GetHashCode() => Value.GetHashCode();

    public override string ToString() => Value.ToString();
    public static bool operator ==(TESTID a, TESTID b) => a.Equals(b);
    public static bool operator !=(TESTID a, TESTID b) => !(a == b);

    public int CompareTo(TESTID other) => Value.CompareTo(other.Value);
}

The source generator would use this template to generate the partial for each of the decorated IDs. It would take care of using the correct namespace, class hierarchy, and the correct ID names etc. by doing a basic replacement of TESTID for DefaultId etc.

To help people get started easily, the source generator could automatically copy the existing "basic" definitions for Guid/int/long/string IDs etc to a project. Consumers can edit these, delete them, or odd new ones as they see fit.

The big advantage is that it's easy for you to completely control how your IDs are generated in your own project, without needing to update the library. You can create as many different templates, as many as you need, for different purposes.

So... what do people think!?

This would obviously be a big change, but I think it will make the library much more flexible for everyone. I've written a PoC, and it all seems to work well, the question is, what do other people think?

Drop a 👍 if you think this sounds like a good idea, or 👎 if you don't. And feel free to drop me a comment!

@Timmoth
Copy link

Timmoth commented Mar 9, 2023

Seems like it would really add a lot of flexibility, though I wonder how much it would affect one of the main pros of this library; is its simplicity.

I might be missing something but being able to just add a package & attribute is much nicer than having to specify default templates if you're not going to need to use them.

@andrewlock
Copy link
Owner Author

I wonder how much it would affect one of the main pros of this library; is its simplicity.

That's my biggest concern too. But one of the related issues currently, is that even you "only" add an attribute, you then have to choose one of (currently) 6 different backing types, followed by a factorial combination of (currently) 3 different interface implementations, and (currently) 6 different converters. That's something like 750 different potential combinations already 😅 And Pretty much every open issue wants to add another option...

I might be missing something but being able to just add a package & attribute is much nicer than having to specify default templates if you're not going to need to use them.

So the goal is to still include default templates for all the current backing types, probably with most things implemented and converters like Json converters etc implemented, but commented out (as they require extra references).

So essentially, the getting started experience should be the same - install the package (which adds the default templates), and add an attribute. And if you want to tweak the generated code, tweak the templates (or add new ones).

The main "icky" thing, is that the package has to dump the templates into your project (so that you can customise them), which is a bit meh. There are other options:

  • Embed the default templates in the generator. The benefit is it's less ugly, but the big downside is that customizing them is much harder
  • Don't add the templates by default, but add an analyzer/code-fix to the attribute to allow generating the template when selected. It looks like stylecop use a similar approach to this here. That's obviously more work involved for this, and it potentially reduces the discoverability, but otherwise it's quite nice

@Antaris
Copy link

Antaris commented Mar 9, 2023

I think this blurs the problem domain somewhat. You're moving away from a simple source generator that solves a specific use-case, to a more generic template-driven source generator which has many applications.

I wonder if you could split it into two packages

  • A template-driven source generator - contains all the nuts and bolts to build a source generator using template conventions (definitions of where templates are read, what variables you might want to support, etc.)
  • A strongly-typed ID source generator built from the template generator - basically just providing a standard set of ID implementations, which is what you've already got.

That way consumers still get the simplicity of strongly-typed IDs using a standard set of base configurations, and then if they need something more, they can use the lower-level template-based generator:

[StronglyTypedId]
public partial struct UserId { }

vs.

[TemplateSourceGenerator("strongly-typed-id-efcore")]
public partial struct UserId { }

Your strongly-typed ID generator is akin to just inheritance:

public sealed class StronglyTypedIdAttribute : TemplateSourceGeneratorAttribute
{
  // Where @templates represents an embedded template?
  StronglyTypedIdAttribute(string template = null) : base(template ?? "@templates/int") { }
}

@tiesmaster
Copy link

First of all, this library is great!! Adding this to an application is really simple, and that simplicity is really what I appreciate about this library.

I agree with some of the others here, that requiring you to also add templates (and the possible maintenance burden with that) to your app would make it more difficult to use, and also convince team members to use this library. I understand that all the feature requests complicate matters a lot for this library, and I definitely agree with you that this all shouldn't be solved by this library. I'd argue that StronglyTypedId should "do the right thing" for the average case (like Newtonsoft, STJ, EFcore serialization, just the top 5 popular ones, perhaps), and all other "snowflake cases" allow through extension.

In short, I would also appreciate a more hybrid approach where this "user templating" would be added as a feature (more or less as @Antaris is suggesting), that gives an "escape hatch" for people that have "snowflake" demands, but still having the StronglyTypedId do the "sane" thing. Interacting with this, like going from "default generator" to "own template based", I'd definitely appreciate the analyzer/codefix approach, as that is getting sort of the standard in this space, and really love it, if a library provides with this.

I'm here to help out, if needed. I have some experience with analyzers/codefixers, so that might come in handy. If there are any "up for grabs" issues, I'd be happy to help out with those, but I can imagine you want to hold that off for now, until you've made a decision on this design proposal.


BTW Whatever you decide on this design, I'd back that up, as only you have the full overview of this maintainability problem.

@egil
Copy link

egil commented Mar 10, 2023

I'm tempted to say "what's the point of your source generator then?". Users should buy into your design decisions, or create their own.

My instinct would be to allow users to customize the type of the underlying ID value, e.g.:

[StronglyTypedId<Guid>()] // .net 7
[StronglyTypedId(typeof(Guid)] 

Perhaps have a constraint that it should be comparable.

@andrewlock
Copy link
Owner Author

Thanks for your thoughts @Antaris, @tiesmaster and @egil!

I think this blurs the problem domain somewhat. You're moving away from a simple source generator that solves a specific use-case, to a more generic template-driven source generator which has many applications.

I mean, you could use it like that I guess, but it would be pretty limited, and clearly focused on this one small use case. @mrpmorris's Moxy library is aiming to fill that niche. This would be similar, but more focused.

I wonder if you could split it into two packages

I'm noping out of that 😅 Definitely don't want to support 2 packages, I'll either carry on with the original approach or switch to the template one (likely with the extra niceties added later).

I agree with some of the others here, that requiring you to also add templates

In case I wasn't clear, you wouldn't have to add templates, you'd only have to add them if you want to customise the output.

I'd argue that StronglyTypedId should "do the right thing" for the average case (like Newtonsoft, STJ, EFcore serialization, just the top 5 popular ones, perhaps),

Trouble is, what's the "right thing"? I feel a bit like it's mostly already there, but there's certain things that are divisive, like impilcit/explicit conversions. There's no right or wrong answer. But adding all these extra options is what is going to bloat the library. I'm not sure where the right stopping point is, hence the template idea.

that gives an "escape hatch" for people that have "snowflake" demands

I wish it were only "snowflakes". My expectation is that basically everyone needs custom converters to use the IDs well, depending on whether you're using efcore, dapper, aspnetcore, swagger etc etc.

I'd definitely appreciate the analyzer/codefix approach

Yeah, I think that would give a nice on-ramp to templates

I'm tempted to say "what's the point of your source generator then?". Users should buy into your design decisions, or create their own.

Creating a source generator is not trivial. Why use any library when you can create your own 😉🤷‍♂️

My instinct would be to allow users to customize the type of the underlying ID value, e.g.:

Yeah, it already does that, and that wouldn't change, it would just be done with templates instead.

@mrpmorris
Copy link

Thanks for the promotion :)

@panoukos41
Copy link

Coming from twitter I too would like to express how awesome this library is 😄!

Now,

Personally I like this desing. I feel it could be even more easier to get started since all customization lives in the same attribute. I don't mind it being a small template engine since this would really allow me to implement my extra-features and most of the issues could definitely be turned into one more included template with the only problem being how do I name it.

The I only thing that crossed my mind as I was reading the proposed desing is it would be great to allow multiple attributes or values so that you could have an additive effect on the generated code.

eg:

I decided I am using the int type and I want ef-core support, the int-ef-core could just add the ef-core bits or int-json would add json.

Even better: If the templates could provide the Value Type just like TESTID an ID_VALUE_TYPE could do the trick, then just the ef-core and json flags would add the necessary code to do the work for each case.

I know the example is obviously not that strong for built-in things but for people that want to extend the functionality It could allow them to add to it without the need of editing & copying the templates and I do believe it would be easier to commit back new templates.

No matter the decision I would still be happy to using the library since it is really awesome and it solves a small but great problem 😁

@andrewlock
Copy link
Owner Author

Thanks for the feedback @panoukos41 🙂

Personally I like this desing. I feel it could be even more easier to get started since all customization lives in the same attribute. I don't mind it being a small template engine since this would really allow me to implement my extra-features and most of the issues could definitely be turned into one more included template with the only problem being how do I name it.

That really gets at the core of what I'm trying to achieve, so great to hear it echoed! Currently if I haven't implemented a feature you need, you fall off a cliff. With the templates, if you need an extra feature you can just add it (and optionally contribute a change to the templates back, which I'm much more likely to accept, as breaking changes pretty much go away)! 🎉

The I only thing that crossed my mind as I was reading the proposed desing is it would be great to allow multiple attributes or values so that you could have an additive effect on the generated code.

I love this idea - I would definitely allow providing multiple values to the attribute 👍 Getting very close to the Moxy mixin approach then 😉

Even better: If the templates could provide the Value Type just like TESTID an ID_VALUE_TYPE could do the trick, then just the ef-core and json flags would add the necessary code to do the work for each case.

It's a nice idea, though I'm not sure it would be feasible unfortunately. As an example, the converter for a string EF Core converter likely needs to be fundamentally different to a converter for Guid or NewId; it's not just a case of switching out the type. It may be worth adding anyway, even if it only works in a couple of cases, given it would be easy to add.

@mrpmorris
Copy link

mrpmorris commented Mar 11, 2023

These are always decorators on a class which add a property, aren't they?

It's a shame we can't add attributes to existing members. I had to write a Fody weaver to achieve that goal.

@andrewlock do you know you can already do this in Moxy? You could literally write one of the templates in a couple of minutes. I might give it a quick go when I next turn on my laptop:)

@andrewlock
Copy link
Owner Author

@andrewlock do you know you can already do this in Moxy? You could literally write one of the templates in a couple of minutes. I might give it a quick go when I next turn on my laptop:)

@mrpmorris yep, totally get that, my idea is that this would essentially be a constrained subset of what Moxy aims to provide.

Somewhat unrelated, but one thing I would kind of like to aim for in the templates is for them to be valid c# on their own, which would mean no moustache syntax. Although it's maybe not as essential for source generators as you get quick feedback anyway 🤔

@mrpmorris
Copy link

I've created a short video showing how to make your own custom Strongly Typed Id classes in a matter of seconds simply by copy/pasting the code from this discussion and doing a quick Search/Replace.

https://youtu.be/eCyKrmJIRVg

If the coder consuming the template wants it slightly different, they can edit the .mixin file and get live feedback as they edit. No endless options needed (as you said).

If you get an error in your mixin, VS will show it as a compiler error, telling you the filename of the mixin file, which line + column, and the compiler error message explaining why it won't compile.

Give it a try, the source code is available here to try : https://github.com/mrpmorris/StronglyTypedIdMixins

@StevenTCramer
Copy link

https://youtu.be/eCyKrmJIRVg

This is awesome.

@lazyWombat
Copy link

lazyWombat commented Apr 18, 2023

It is possible to have a StronglyTypedId based on the existing type instead of the text template?

[StronglyTypedId<MyTemplateId>]
public partial struct UserId { }

readonly partial struct MyTemplateId : System.IComparable<Guid-or-whatever-type>, System.IEquatable<Guid-or-whatever-type> { ... }

@markusschaber
Copy link

I've created a short video showing how to make your own custom Strongly Typed Id classes in a matter of seconds simply by copy/pasting the code from this discussion and doing a quick Search/Replace.

https://youtu.be/eCyKrmJIRVg

It seems that Moxy, the tool used, currently does not support structs, so we cannot use it :-)

@mrpmorris
Copy link

@markusschaber Fixed in 1.4 (just released)

Thanks for letting me know!

@petrkoutnycz
Copy link

I like what @egil said regarding generics.

I also really like simplicity of this library and do not expect it to solve every problem on Earth. The only thing I miss at the moment is the New() method for Guid to return sequential guid instead of Guid.NewGuid() so I can optimize persistence.

@andrewlock
Copy link
Owner Author

FYI, I (finally) got to the work for this in #117. I'm pretty sure this is the way I'm going to go, as it strikes the right balance between opinionated and flexible IMO. Plus it means I can close 30+ issues 😉

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

Successfully merging a pull request may close this issue.