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

Add @code directive support for Blazor #527

Merged
merged 2 commits into from
May 2, 2019
Merged

Conversation

ajaybhargavb
Copy link
Contributor

Fixes dotnet/aspnetcore#6361

  • Added @code directive that has the exact behavior as @functions
  • Conditionally adding the directive only for Blazor
  • Updated tests
  • Verified it works in VS with the correct directive completion

Note:

  • I didn't remove @functions directive support in Blazor.
  • There might be weird problems if people use both functions and code in the same file. Should we add any restrictions/warnings here?

Copy link
Contributor

@NTaylorMullen NTaylorMullen left a comment

Choose a reason for hiding this comment

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

Oo fancy. I have mixed feelings about having @code and @functions simultaneously available in component files but I think overall it's a good option to have so we're not completely different from classic Razor.

@NTaylorMullen
Copy link
Contributor

  • There might be weird problems if people use both functions and code in the same file. Should we add any restrictions/warnings here?

What sort of problems?

@ajaybhargavb
Copy link
Contributor Author

ajaybhargavb commented May 1, 2019

What sort of problems?

Hello World
@functions { // Some code 1 }
@code { // Some code 2 }
@functions { // Some code 3 }

assuming functions directive pass runs first followed by code directive pass the above will be transformed into,

- Namespace
  - class
    - method
    - // Some code 1
    - // Some code 3
    - // Some code 2

Note the change in the ordering. This could potentially cause problems.
But I know this is an implementation details and a weird corner case. But my question was mostly whether we should even support the above scenario.

@NTaylorMullen
Copy link
Contributor

I could see that being an issue for extremely large Razor documents and to me what makes it a corner case is the intermingling of functions and code. My bet is that most people will either use functions or they'll use code, super rarely both.

So ya, I agree with you that it's a weird corner case. Assuming it's a minor amount of work to implement your idea of adding warnings/restrictions. Which would you do?

@ajaybhargavb
Copy link
Contributor Author

My bet is that most people will either use functions or they'll use code, super rarely both.

Exactly. Which is why I was wondering if we should enforce that in some way.

Assuming it's a minor amount of work to implement your idea of adding warnings/restrictions. Which would you do?

I think the easiest thing to do to avoid this completely is to merge functions and code directive passes into one.

@NTaylorMullen
Copy link
Contributor

I think the easiest thing to do to avoid this completely is to merge functions and code directive passes into one.

Sounds fine to me. Would keep the logic consistent amongst the two yet still give us opportunities to diverge the functionality in the future if we want. Only thing to watch out for is we'd need to support users registering just one of the directives (or both) manually so we'd need to be sure that scenario works.

@ajaybhargavb
Copy link
Contributor Author

🆙 📅


namespace Microsoft.AspNetCore.Razor.Language.Components
{
public static class ComponentCodeDirective
Copy link
Contributor

Choose a reason for hiding this comment

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

Name this CodeDirective and the Register below as RegisterForComponents

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just following the same pattern as other component directives.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see, didn't realize we did that. Leave for now then, something we should revisit for all of the component like directives before RTM

}

// Now we have all the directive nodes, we want to add them to the end of the class node in document order.
var orderedDirectives = directiveNodes.OrderBy(n => n.Source?.AbsoluteIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

@ajaybhargavb ajaybhargavb merged commit 1a4b0cb into master May 2, 2019
@ajaybhargavb ajaybhargavb deleted the ajbaaska/code-directive branch May 2, 2019 00:18
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.

Add support for @code directive
2 participants