Skip to content

IFileEntryOwner: Prevent ConstructorContainsNullParameterNames when AOT Compiled #6032

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

Conversation

mtbayley
Copy link
Contributor

Description

Closes #6029

How Has This Been Tested?

Tested with Demo App

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality not to work as expected)

Checklist:

  • The PR is submitted to the correct branch (master for dev, rel-1.x for maintenance).
  • My code follows the code style of this project.
  • I've added relevant tests.

@stsrki stsrki requested a review from tesar-tech March 19, 2025 15:19
@tesar-tech
Copy link
Contributor

To summarize how the bug works (for me and for future reference):

It starts here in js:

adapter.invokeMethodAsync('NotifyChange', fileList).then(null, function (err) {

which calls a dotnet method, implemented here:

public Task NotifyChange( FileEntry[] files )

FileEntry contains the property IFileEntryOwner Owner, which has this property:

IJSFileModule JSFileModule { get; set; }

This fails to be marked as "unable to be deserialized" due to AOT trimming.

Is that correct?


The bug fix works, yes. We can merge it. But I don't like the solution:

  • There is [JsonIgnore] appearing out of nowhere within an interface that isn’t supposed to touch JSON at all.
    • Someone will probably (rightfully) question why it’s there.
  • How do we know no other properties are causing the same issue? (There’s probably a way, but you really don’t want to have to keep that in mind.)

I suggest creating FileEntryDto to clearly define what should and shouldn’t be transferred between js and dotnet.

Similar issues exist on the js side, where some "hacks" are used to avoid serialization:

Object.defineProperty(fileEntry, 'blob', { value: file });

So we might want to solve it on both sides.

Not part of this PR—we can move this to another ticket.


@mtbayley Can you add a simple comment explaining why the attribute is there?

btw thanks for this and for the referenced issue, I had no idea such issues might happen.

Copy link
Contributor

@tesar-tech tesar-tech left a comment

Choose a reason for hiding this comment

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

just the comment

@mtbayley
Copy link
Contributor Author

mtbayley commented Mar 19, 2025

@tesar-tech Yes, you understand the issue correctly. I have run into a similar issue with my AuthenticationStateProvider in my work.

[JsonIgnore] tells System.Text.Json to not bother serializing/deserializing during the NotifyChange invocation.

If you have a better solution, I'm up for testing that as well. I agree, the error only occurs during runtime for a released AOT compiled app. It would be hard to catch during development or testing.

@tesar-tech
Copy link
Contributor

@stsrki what's the strategy?

  • merge the fix, create solution for 1.8 (or another pr)
  • merge the fix and leave it
  • create the solution right here.

@stsrki
Copy link
Collaborator

stsrki commented Mar 20, 2025

I think we don't even need it. Why can't we remove JSFileModule from IFileEntryOwner? And just call await JSFileModule.RemoveFileEntry( ElementRef, fileEntry.Id ); directly from FileEdit and Markdown?

Or, change RemoveFileEntry as

async Task RemoveFileEntry( IJSFileModule module, IFileEntry fileEntry, CancellationToken cancellationToken = default )
{
    module.RemoveFileEntry( ElementRef, fileEntry.Id );
}

@tesar-tech
Copy link
Contributor

That will just eliminate the purpose of the interface and duplicate the code. The shared functionality is there, because it's the same for both of the classes.

And it wont solve the "might happen for other types that should not be in dto" issue.

@stsrki
Copy link
Collaborator

stsrki commented Mar 21, 2025

The thing is, we shouldn't have introduced a new API in the 1.7 release. We broke our own rules, basically. At the time, I had wrongly assumed it was not a big change, so it wouldn't hurt. So I would rather to duplicate code then introducing JSON attribute to the interface.

We can optimize it in 1.8.

@stsrki stsrki requested a review from tesar-tech March 21, 2025 15:28
@tesar-tech
Copy link
Contributor

  • It doesn't change the API - the JSFileModule is ignored either way. The attribute just confirm that on AOT.
  • I suspect the IFileEntryOwner is more like the "private public" blazorise interface than really an interface for users to utilize.
  • I really don't think someone uses the IFileEntryOwner.
  • Even if someone does use the IFileEntryOwner, I don't really think someone is eager to serialize the IJSFileModule. Maybe it is even not possible.

The [JsonIngore] attribute won't affect anyone in a negative way.

We can optimize it in 1.8.

sounds like you don't really care. If that's the case, I have no objections against the latest changes.

@stsrki stsrki changed the title Prevent ConstructorContainsNullParameterNames when AOT Compiled IFileEntryOwner: Prevent ConstructorContainsNullParameterNames when AOT Compiled Mar 25, 2025
@stsrki stsrki mentioned this pull request Mar 25, 2025
@stsrki stsrki merged commit f80e00b into Megabit:rel-1.7 Mar 25, 2025
1 check passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 25, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants