Skip to content

Supply "IsConnected" state info to components #8888

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 4 commits into from
Mar 29, 2019

Conversation

SteveSandersonMS
Copy link
Member

This is a partial implementation of what we need for #8786. It's pretty basic and just follows from existing patterns.

Still to do: E2E tests showing the swap from "prerendering/disconnected" into "connected", with a component that renders different output in the two cases.

Question: Do we need some other flag to differentiate the "prerendering" case from the "was interactive but then disconnected" case? I'm not sure we have a use case for that.

// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

namespace Microsoft.AspNetCore.Components.Services
Copy link
Member

Choose a reason for hiding this comment

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

What would you think about putting this in Microsoft.AspNetCore.Components. I really think we'll end up removing .Services as a namespace when this gets reviewed in favor of some mix of:

  • moving these types into .Components
  • moving these types into more specific namespaces like routing/layouts
  • moving them into a more generic namespace like .Infrastructure for things that most people won't use

Copy link
Member Author

Choose a reason for hiding this comment

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

Totally happy to merge the Components.Services namespace into .Components. That would also fix one of the other things that's been bothering me lately - that the templates imports don't include .Services.

Filed #8892

/// <example>During prerendering, the value will always be false.</example>
/// <example>During server-side execution, the value can be true or false depending on whether there is an active SignalR connection.</example>
/// <example>During client-side execution, the value will always be true.</example>
bool IsConnected { get; }
Copy link
Member

Choose a reason for hiding this comment

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

Should we document some guarantees about what happens when transition between states? Or is this not the right place for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

What kind of guarantees do you have in mind?

Copy link
Member

Choose a reason for hiding this comment

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

Like, if you weren't connected when you last rendered - does that mean that you get called again when you are connected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see. Since we're not proposing to do that, I'm not sure what there would be to say about it here.

I expect you know already, but the intent to deal with the mainstream "transition from prerendered to interactive" case is via lifecycle methods. For a more general "notify me when connection state changes", I'd expect that to be a separate API, which someone could wrap into a cascading value if they wanted.

Copy link
Member

Choose a reason for hiding this comment

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

Ok cool. I guess what I was thinking about is where are we going to document to the pattern for using this property effectively. That's probably not a good fit for the in-code docs, so feel free to ignore.

@Eilon Eilon added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 28, 2019
@SteveSandersonMS SteveSandersonMS force-pushed the stevesa/icomponentcontext branch from 69dbebe to 6df3556 Compare March 29, 2019 12:03
@SteveSandersonMS
Copy link
Member Author

Temporarily rebased onto #8910 so that the E2E test can work.

@javiercn, I know you already approved this, but now I've added the E2E test it might be worth taking another look. Maybe the format of test will be useful in your prerendering PR too (which I will read this afternoon).

@javiercn
Copy link
Member

I already included something similar in my draft PR :(

endpoints.MapFallbackToPage("/PrerenderedHost");
endpoints.MapComponentHub<TestRouter>("app");
});
});
Copy link
Member

Choose a reason for hiding this comment

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

This should not be our experience for having multiple entry points as part of an app. But that requires a separate discussion. See #8913

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, this is just what you have to do today, not a claim that it's the ideal dev experience or that we shouldn't change it.

@@ -836,6 +836,10 @@ public enum NavLinkMatch
}
namespace Microsoft.AspNetCore.Components.Services
{
public partial interface IComponentContext
Copy link
Member

Choose a reason for hiding this comment

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

How did you update this? Can you try and get the private members of the components that we ship to show up? That breaks the current E2E test in ProjectTemplates. It would be incredibly dope if we updated and enable that.

Copy link
Member Author

@SteveSandersonMS SteveSandersonMS Mar 29, 2019

Choose a reason for hiding this comment

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

I ran GenerateReferenceSource as per https://github.com/aspnet/AspNetCore/blob/master/docs/ReferenceAssemblies.md

What you see here is what it outputs. You can't make it output private members as they are not an aspect of reference assemblies.

Copy link
Member

Choose a reason for hiding this comment

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

I do believe we did this for some other APIs in the assembly. This is an ongoing problem on the templates. Things like Assembly="typeof(Blah).Assembly" won't work because it won't be able to find the Assembly property on the component and it will treat it as a string instead when compiling.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, @rynowak fixed that by telling the tool not to generate anything for those types, and he wrote those ref types manually. I don't think we need to do IComponentContext manually though.

Copy link
Member

Choose a reason for hiding this comment

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

For now we need to manually generate the reference source of components because non-public methods do not show up in reference assemblies. This should not apply to anything else we need to do. If the contagion starts spreading I would love to learn more about that 😆

Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Signed off again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants