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

Support for custom primitive types #1498

Merged
merged 35 commits into from
Aug 3, 2023
Merged

Conversation

tomasherceg
Copy link
Member

We've added support for registering custom primitive types.

There is one unresolved issue - the static command wasn't deserializing method arguments using converters from DotVVM.
We've introduced this, but:

  1. It is a breaking change.
  2. The code needs to be refactored as the argument deserialization is in DotvvmPresenter that should not depend on Newtonsoft.Json. I think it should be moved to IViewModelSerializer, or maybe added as another interface.

Also, unit tests and UI tests are missing.

@exyi
Copy link
Member

exyi commented Nov 1, 2022

There is one unresolved issue - the static command wasn't deserializing method arguments using converters from DotVVM

Awesome, this is the subject of #809. We will have to leave it for 5.0, since this breaks quite many things. Or maybe put it behind a feature flag.

public class CustomPrimitiveTypeAttribute : Attribute
{
/// <summary>
/// Gets a type which will be used for this custom type on the client side. Only types from ReflectionUtils.PrimitiveTypes (or their nullable versions) are supported.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This type can not be actually nullable. We handle those differently

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the problem with nullable types here? In the sample, we are using Guid? since the typed id is a reference type and can be null.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This use-case seems to work nicely with reference types. If SampleId, which is a reference type, maps to Guid? - then properties that are of type SampleId work correctly.

The problem comes when SampleId is a value type (struct). Then we can have also a nullable and non-nullable property. This can generate funny stuff like this:
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add a check when generating metadata for value types so that such situation can not happen. Otherwise users should not registrate these with nullable underlying type, otherwise it can be doubled.

src/Framework/Framework/Utils/ReflectionUtils.cs Outdated Show resolved Hide resolved
@tomasherceg tomasherceg added this to the Version 4.1 milestone Nov 23, 2022
@@ -17,11 +17,15 @@ public class DotvvmExperimentalFeaturesConfiguration
[JsonProperty("explicitAssemblyLoading")]
public DotvvmGlobalFeatureFlag ExplicitAssemblyLoading { get; private set; } = new DotvvmGlobalFeatureFlag();

[JsonProperty("useDotvvmSerializationForStaticCommandArguments")]
public DotvvmGlobalFeatureFlag UseDotvvmSerializationForStaticCommandArguments { get; private set; } = new DotvvmGlobalFeatureFlag();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to UseDotvvmSerializationForStaticCommands
Add XML comment
Will be true by default in the next release
Make sure it applies also for static command response

@acizmarik
Copy link
Member

We just discussed with @exyi that there is another potential issue - when user creates a custom primitive type and binds it into a textbox. How to treat this expression?

  • We said that on client-side we would use the underlying types (which might have special formatting)
  • However, expressions are generated on server from the custom type (that does not have special formatting by default)

I am not sure that this behaviour is clear or generally expected by users. What is the correct approach here?

@acizmarik
Copy link
Member

Furthermore, format overriding is not trivial (see our textbox binding handler 😄). The easiest solution would be IMO to format based on the underlying type

@tomasherceg
Copy link
Member Author

user creates a custom primitive type and binds it into a textbox
For now, I would not support this. We'd have to provide ToString(format) implementation and JS translation, and somehow extend the globalize library to support formatting.
If these types don't work with TextBox, we should throw a compilation error.

@tomasherceg tomasherceg force-pushed the feature/custom-primitive-types branch from f3385e2 to 48119e0 Compare December 14, 2022 10:17
@tomasherceg tomasherceg removed this from the Version 4.1 milestone Dec 21, 2022
@exyi exyi added this to the Version 4.2 milestone Jan 3, 2023
@exyi
Copy link
Member

exyi commented Jun 19, 2023

One more small issue - Currently, the validator skips all primitive types. I'd expect it to be validated if the custom primitive implements IValidatableObject, or if the value property has a ValidationAttribute.

@tomasherceg tomasherceg force-pushed the feature/custom-primitive-types branch from 696a647 to 876d386 Compare July 30, 2023 09:09
@tomasherceg
Copy link
Member Author

The last thing we wanted to do is to change the [CustomPrimitiveType] attribute to a marker interface. Its presence is easier to detect without the need for caching.

exyi added 4 commits August 1, 2023 21:44
This allows for efficient checks if a value is primitive
without the need for cache hashtable containing all types we encounter
…interface

Switch custom primitive types to a marker interface
@exyi exyi merged commit 608b96a into main Aug 3, 2023
@exyi exyi deleted the feature/custom-primitive-types branch August 3, 2023 19:22
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.

3 participants