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

[VS Code] Register compiler features in MVC 3.0 projects #8061

Merged

Conversation

allisonchou
Copy link
Contributor

Summary of the changes

  • I have no idea how this was working before, but essentially, I discovered a new bug in VS Code where components aren't being discovered. After digging, this turns out to be because we aren't registering compiler features in our MVC 3.0 ProjectEngineFactory.
  • This breaks in VS Code and not VS because somehow VS is still using the legacy MVC 3.0 ProjectEngineFactory, which does include registering compiler features. VS Code is using the non-legacy variation.
  • I believe the proper fix here is to add registering compiler features to the non-legacy MVC 3.0 ProjectEngineFactory. However, I don't have a ton of context in this area, so additional insight is welcome.

Before:
image

After:
image

@allisonchou allisonchou marked this pull request as ready for review January 3, 2023 08:32
@allisonchou allisonchou requested a review from a team as a code owner January 3, 2023 08:32
public override RazorProjectEngine Create(
RazorConfiguration configuration,
RazorProjectFileSystem fileSystem,
Action<RazorProjectEngineBuilder> configure) => Create(configuration, fileSystem, configure, registerCompilerFeatures: true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main part of the change (registerCompilerFeatures: true).

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, I'll be this was really annoying to track down.

Copy link
Contributor

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me, though I have no context on ProjectEngineFactory stuff since I don't think it's been touched in years. I'm not sure who would be best for that, maybe the compiler folks or @NTaylorMullen might remember some context.

public override RazorProjectEngine Create(
RazorConfiguration configuration,
RazorProjectFileSystem fileSystem,
Action<RazorProjectEngineBuilder> configure) => Create(configuration, fileSystem, configure, registerCompilerFeatures: true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, I'll be this was really annoying to track down.

@allisonchou
Copy link
Contributor Author

After talking briefly with Taylor, it seems that this may not be the ideal long-term fix. However, I think it should suffice for now after manually testing out various MVC scenarios (netcoreapp3.1/6.0/7.0) and having things work as expected.

The long-term fix would probably involve DefaultProjectEngineFactory, which somehow isn't utilized at all by VS or VS Code today. I filed #8064 to track future investigation.

@allisonchou allisonchou merged commit 24e61d8 into features/UpdateVSCodeExtension Jan 3, 2023
@allisonchou allisonchou deleted the allichou/FixProjectEngineFeatures branch January 3, 2023 23:49
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