Skip to content

Replace AssignToProperties with SetParameterProperties, which also clears unspecified parameter properties (imported from Blazor PR 1108) #4797

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

Merged
merged 5 commits into from
Dec 14, 2018

Conversation

SteveSandersonMS
Copy link
Member

Imported from dotnet/blazor#1108


<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<Description>Components feature for ASP.NET Core.</Description>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
Copy link
Member Author

Choose a reason for hiding this comment

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

This is because this PR adds a usage of stackalloc, and prior to netstandard2.1, that is always treated as unsafe.

Once we go to netstandard2.1 the unsafe modifier can be removed.

Copy link
Member

Choose a reason for hiding this comment

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

hmm, I think that's due to the language version not the TFM right? Either way I'm not too nervous about this as long as it works in wasm.

@SteveSandersonMS
Copy link
Member Author

@rynowak Another one.

In this specific case it's worth being aware of the low-level thing I've added with an unsafe method - please let me know if you think that code is unreasonable or will violate any shipping rules.


// TODO: Once we're able to move to netstandard2.1, this can be changed to be
// a Span<bool> and then the enclosing method no longer needs to be 'unsafe'
bool* usageFlags = stackalloc bool[numWriters];
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see now.

parameterWriters = CreateParameterWriters(targetType);
_cachedParameterWriters[targetType] = parameterWriters;
writers = new WritersForType(targetType);
_cachedWritersByType[targetType] = writers;
Copy link
Member

Choose a reason for hiding this comment

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

nit: changing this to TryAdd is a little more friendly and makes the pattern obvious.

@RemiBou
Copy link
Contributor

RemiBou commented Dec 14, 2018

What should I do about failing e2e test ? It's because it calls a page without the date parameter so the date property get date default value. I can handle it by overriding OnParameterSet I think.

@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/import-blazor-pr-1108 branch from 8cf1594 to 4175517 Compare December 14, 2018 09:09
@SteveSandersonMS
Copy link
Member Author

What should I do about failing e2e test

@RemiBou Good point - I've added a fix for that.

@SteveSandersonMS SteveSandersonMS merged commit 3432083 into master Dec 14, 2018
@SteveSandersonMS SteveSandersonMS deleted the stevesa/import-blazor-pr-1108 branch December 14, 2018 17:07
@RemiBou
Copy link
Contributor

RemiBou commented Dec 14, 2018

Thanks @SteveSandersonMS . I think you changed pretty much all my code, but it's for the best and it'll help me greatly in the future !

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