Skip to content

~/ should result in error in local functions and @functions #10734

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

Closed
NTaylorMullen opened this issue Jun 1, 2019 · 3 comments
Closed

~/ should result in error in local functions and @functions #10734

NTaylorMullen opened this issue Jun 1, 2019 · 3 comments
Assignees
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@NTaylorMullen
Copy link

Because of how ~/ is implemented today (as a TagHelper) it requires the method that contains it to be asynchronous. This means when utilizing ~/ in local functions, lambdas etc. you must mark those methods as async Task. Sadly, ~/ does not show up at design time as a TagHelper so you don't get errors for this scenario.

All the being said, we should consider moving ~/ off of the TagHelper infrastructure and make it a compiler intrinsic.

Filing this issue because #8630 did not cover the ~/ scenario.

@NTaylorMullen NTaylorMullen added enhancement This issue represents an ask for new feature or an enhancement to an existing one area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels Jun 1, 2019
@NTaylorMullen NTaylorMullen self-assigned this Jun 1, 2019
@mkArtakMSFT mkArtakMSFT added this to the 3.0.0-preview7 milestone Jun 4, 2019
@NTaylorMullen
Copy link
Author

So I've been investigating multiple ways to do this over the past several days all of which have hit various roadblocks all exposing different issues with fulfilling this issues core requirement. The core requirement for this feature is: Work at least as well as the UrlResolutionTagHelper works today.

To make that possible we need to support:

  1. Plain HTML attributes, <img src="~/some/path.png" />
  2. Mixed HTML attributes, <img src="~/@somePath more stuff" />
  3. TagHelper bound attributes, (imagine there's another ImageTagHelper with public string Src { get; set; }), <img src="~/some/path.png" />
  4. TagHelper unbound attributes <img src="~/some/path.png" />

Less important but possible today:

  1. HTML attributes with statements, <img src="~/@somePath @if (true) { @extra } " />
  2. Ignores non-standard ~/ attributes, <randomTag something="~/path/to/somewhere" />

So we're left with a few options:

Option 1

Stop supporting all that UrlResolutionTagHelper does today. Only support 1, 3 and 4 from above.

Pros: Lesser implementation/maintenance cost
Cons: Will make it more difficult to port some applications because of differing behavior from older versions of Razor.

Option 2

Buffer the world. Everytime we see a ~/ build a new system to buffer values and then resolve their app relative URL after rendering.

Pros: Keeps consistency with previous versions of Razor
Cons: Lots of work, hard to maintain, not very performant.

Option 3

Leave ~/ resolution async and have it show up in VS as a TagHelper to provide valid errors in markup blocks.

Pros: Easy to implement and maintain
Cons: TagHelpers still have to pay the cost to go async and any methods that contain a ~/ valid attribute must be async.

Option 4

Do nothing
Pros: lol
Cons: ¯\(ツ)


@mkArtakMSFT @danroth27 @rynowak @ajaybhargavb Would appreciate your input here. I personally am leaning towards 3 or 4 to avoid the more common behavior changes of 1 and 6.

@NTaylorMullen
Copy link
Author

Talked offline with @rynowak. For 3.0.0 we're going to investigate what it means to go with Option 3

@NTaylorMullen
Copy link
Author

Updated the cost to reflect the potential that we might need to perform some VS changes to make this plausible.

NTaylorMullen pushed a commit that referenced this issue Jun 24, 2019
- This change results in proper errors for tags that utilize `~/`inside of local functions at design time.

#10734
NTaylorMullen pushed a commit that referenced this issue Jun 26, 2019
- This change results in proper errors for tags that utilize `~/`inside of local functions at design time.
- Updated TagHelper in code blocks analyzer error to mention `~/` since it will typically be a cause of confusion.

#10734
NTaylorMullen pushed a commit that referenced this issue Jul 2, 2019
- This change results in proper errors for tags that utilize `~/`inside of local functions at design time.
- Updated TagHelper in code blocks analyzer error to mention `~/` since it will typically be a cause of confusion.

#10734
@NTaylorMullen NTaylorMullen added Done This issue has been fixed and removed Working labels Jul 2, 2019
NTaylorMullen pushed a commit that referenced this issue Jul 2, 2019
- This change results in proper errors for tags that utilize `~/`inside of local functions at design time.
- Updated TagHelper in code blocks analyzer error to mention `~/` since it will typically be a cause of confusion.

#10734
@NTaylorMullen NTaylorMullen changed the title ~/ should not require async ~/ should result in error in local functions and @functions Jul 23, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

2 participants