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

Investigate issue with TimeTagHelper #2089

Closed
stevejgordon opened this issue Aug 25, 2017 · 4 comments · Fixed by #2198
Closed

Investigate issue with TimeTagHelper #2089

stevejgordon opened this issue Aug 25, 2017 · 4 comments · Fixed by #2198

Comments

@stevejgordon
Copy link
Member

After 2.0.0 upgrade the TimeTagHelper was presenting strange issues and appeared to be invoked for the head and body tags which led to malformed output.

A workaround was put in to prevent this - see https://github.com/HTBox/allReady/blob/master/AllReadyApp/Web-App/AllReady/TagHelpers/TimeTagHelper.cs#L35

We should investigate the root cause and look to fix this. Copying the tag helper code into a clean project it seemed to work correctly, so this appears to be something specific to the allReady code base.

@mk0sojo
Copy link
Contributor

mk0sojo commented Sep 6, 2017

Tried to have a look at this and it appears to be a problem with Autofac and more precisely:

containerBuilder.RegisterAssemblyTypes(typeof(Startup).GetTypeInfo().Assembly).AsImplementedInterfaces();

To test this create a new Asp.net core 2.0 web app. Create the tag helper and register it. Then add the following code to the end of ConfigureServices in Startup.cs:

        var containerBuilder = new ContainerBuilder();
        containerBuilder.RegisterAssemblyTypes(typeof(Startup).GetTypeInfo().Assembly).AsImplementedInterfaces();
        containerBuilder.Populate(services);

        this.ApplicationContainer = containerBuilder.Build();
        return new AutofacServiceProvider(this.ApplicationContainer);

If the containerBuilder.RegisterAssemblyTypes... row is commented out it will work, otherwise crash.

I haven't been able to figure out why this causes the app to crash yet, but will try to investigate further.

@mk0sojo
Copy link
Contributor

mk0sojo commented Jan 3, 2018

Finally had some time to look closer at this.

The problem comes from a new feature (bug?) in ASP.Net Core 2.0 where you can register a TagHelper with dependency injection to be injected to the head or body of a page. Microsoft does this with the application insight javascript code. More information here aspnet/Mvc#5728.

This causes a problem for us because of this line of code in Services.cs:

containerBuilder.RegisterAssemblyTypes(typeof(Startup).GetTypeInfo().Assembly).AsImplementedInterfaces();

This registers every concrete class in the Allready assembly as themselves and this includes TimeTaghelper and TimeZoneNameTagHelper. To solve this we can exclude classes implementing TagHelper from being injected by changing the code to:

containerBuilder.RegisterAssemblyTypes(typeof(Startup).GetTypeInfo().Assembly).Where(t => !t.IsAssignableTo<TagHelper>()).AsImplementedInterfaces();

@stevejgordon
Copy link
Member Author

Thanks for tracking this down @mk0sojo. I figured it was something to do with the change in 2.0 around the head injection, but for the life of me couldn't work out why. Hadn't considered this global DI registration even existed.

I'll take your PR for a spin locally but this sounds like a good fix. I'm also curious to expand on why we need to register everything this way. I prefer more controlled and explicit registrations which we might be able to review.

@MisterJames
Copy link
Contributor

Yeah...I think this just dates back to perhaps what was available when the project was conceived. Remember that the roots here predate most IoC containers coming over to Core, much less being mature. This is solved in PR #2198 so we can close this out. Thanks folks!

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 a pull request may close this issue.

3 participants