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

refactor: Migrate SwapStyles enum to SwapStyles strongly typed object #49

Closed
wants to merge 7 commits into from

Conversation

tanczosm
Copy link
Collaborator

@tanczosm tanczosm commented May 14, 2024

Problem

Before hitting 1.0 final one of the odd things that is going to pop up is using the swap style builder as a helper for hx-swap attributes. At the moment it will work for hx-swap attributes that utilize the builder but if you just use the enum it will provide incorrect string conversions.

<div hx-get="/home" hx-swap=@SwapStyle.OuterHTML>  -- doesn't work because enum with conversion is "OuterHTML"
<div hx-get="/home" hx-swap=@SwapStyle.OuterHTML.ScrollFocus()>  -- does work because the builder returns proper strings

So the alternative is to use the defined constants as you did a few times:

<div hx-get="/home" hx-swap=@SwapStyles.OuterHTML> // or
<div hx-get="/home" hx-swap=@Constants.SwapStyles.OuterHTML>

But then if you later try to extend the swapstyle using the builder you won't get any intellisense because the builder uses SwapStyle rather than SwapStyles.

Proposed Solution

I created a strongly-typed object called SwapStyle to replace the SwapStyle enum that creates an object with an underlying string and numeric value that works identically to the enum. The downside is that as a complex object it's not a compile time constant that can be used in method signatures, but the only defaults that impacted were SwapStyle.Default, which itself an empty string. I changed those parameters to be nullable and used SwapStyle.Default if the parameter was null.

Pros

  • Allows for consistent usage throughout razor markup and in razor component parameters with and without the builder
  • None of the tests had to be altered to work with this solution.
  • ToHtmxString extension method is no longer necessary

Cons

  • Not a compile time constant that can be used in method signatures

For the cons side, this can be used as a workaround if used as a method parameter:

public void SomeMethod (string style = Constant.SwapStyles.InnerHTML) {}

SomeMethod(SwapStyle.InnerHTML)

This all works:

var style = SwapStyle.OuterHTML;
style = SwapStyle.BeforeBegin;
style = Constants.SwapStyles.Delete;  // implicit string conversion of a string style to the typed value

if (style == SwapStyle.Delete) // true
{
}

if (style == "delete") // also true
{

}

tanczosm added 3 commits May 13, 2024 22:09
Significant updates have been made to the SwapStyle class and its usage across the codebase. The SwapStyle enum has been refactored into a class with static readonly instances for each swap style. This change also includes the addition of a custom JsonConverter for SwapStyle, replacing the previous use of JsonCamelCaseStringEnumConverter.

The Constants.cs file has been updated to reflect correct casing in constant string values. Changes in HtmxConfig.cs, HtmxResponse.cs, and SwapStyleBuilder.cs are made to replace 'is' checks with '==' for comparing swap styles.

A new file, SwapStyleJsonConverter.cs is added to handle serialization/deserialization of the newly refactored SwapStyle class. The old extension method file (SwapStyleExtensions.cs) providing ToHtmxString() is removed as it's no longer needed due to changes in how swap styles are represented.
Removed two ReSharper directives that were suppressing warnings for conditions that are always true or false. These comments were not needed and have been removed to clean up the code.
To improve consistency of usage move to use SwapStyle objects instead of defined constants
@tanczosm tanczosm marked this pull request as ready for review May 14, 2024 12:09
tanczosm added 4 commits May 14, 2024 12:40
Simplified the implementation of the == operator and Equals method in the SwapStyle class by using EqualityComparer and direct comparison respectively. This makes the code more concise without changing its external behavior.
Changed the casing of 'BeforeBegin', 'AfterBegin', 'BeforeEnd', and 'AfterEnd' constants in Constants.cs
@egil
Copy link
Owner

egil commented May 14, 2024

Yeah, it's a bit of a mess right now with too many ways to do the same thing.

I do like the builder, especially for complex swaps with modifiers, but I also want to be performance-conscious, and the builder is quite expensive compared to just using enum or constants.

The enum option has the advantage of not being "stringly-typed", but will require a small modification to the renderer such that if it is used directly in an attribute, it produces the correct markup.

E.g.: [email protected] renders as hx-swap="beforebegin".

That is a simple addition I can make to the renderer.

The SwapStyle enum can also be used as a starting point for the builder, which it already is, such that users can start with the enum and "pay the cost" of using the builder if they need the additional help.

I am in favor of changing the constants we that overlap with enums to internal, to make the API simpler for the end user.

@tanczosm
Copy link
Collaborator Author

I like that idea better if you can mark constants internal and alter the renderer.

@egil
Copy link
Owner

egil commented May 14, 2024

I'll push a commit later that does this.

@tanczosm tanczosm closed this May 14, 2024
@tanczosm tanczosm deleted the feat-type-safe-oop-swapstyle branch May 14, 2024 22:37
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 this pull request may close these issues.

2 participants