Skip to content

Blazor API Review: IUriHelper #12425

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.

Sign up for GitHub

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

Jump to bottom
Merged
rynowak merged 8 commits into from
Aug 2, 2019
Merged

Blazor API Review: IUriHelper #12425

rynowak merged 8 commits into from
Aug 2, 2019

Conversation

@rynowak
Copy link
Member

@rynowak rynowak commented Jul 22, 2019

See: #12397

The last commit is me just doing a bunch of random simplication and clean-up. Let's dicuss anything you don't like about it. I don't have my heart set on doing all of these things, I just had some spare time.

@rynowak rynowak force-pushed the rynowak/uri-helper branch from c8b2caf to 9250334 Compare July 22, 2019 02:59
javiercn
javiercn reviewed Jul 22, 2019
View reviewed changes
javiercn
javiercn reviewed Jul 22, 2019
View reviewed changes
javiercn
javiercn reviewed Jul 22, 2019
View reviewed changes
@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Jul 22, 2019

This looks like a lot of good improvements to me. I particularly like the new name and the fact that things are properties instead of Get...() methods now.

campersau
campersau reviewed Jul 22, 2019
View reviewed changes
@Pilchie Pilchie added the area-blazor Includes: Blazor, Razor Components label Jul 22, 2019
pranavkm
pranavkm reviewed Jul 23, 2019
View reviewed changes
@rynowak rynowak force-pushed the rynowak/uri-helper branch from 2ae5427 to cf5f4dd Compare July 26, 2019 20:37
@mkArtakMSFT mkArtakMSFT added area-blazor Includes: Blazor, Razor Components tell-mode Indicates a PR which is being merged during tell-mode and removed area-blazor Includes: Blazor, Razor Components labels Jul 30, 2019
@rynowak rynowak force-pushed the rynowak/uri-helper branch from cf5f4dd to a26d098 Compare July 31, 2019 02:24
dougbu
dougbu reviewed Jul 31, 2019
View reviewed changes
@rynowak rynowak changed the base branch from master to release/3.0 July 31, 2019 18:52
@rynowak
Copy link
Member Author

rynowak commented Jul 31, 2019

Is there a good reason for this type to be public? Presumably we want users to program against the NavigationManager instance?

Because of JS interop. I'm planning to fix it.

@rynowak rynowak force-pushed the rynowak/uri-helper branch from a26d098 to da83ed2 Compare July 31, 2019 23:06
@rynowak rynowak requested a review from a team as a code owner July 31, 2019 23:06
@rynowak
Copy link
Member Author

rynowak commented Jul 31, 2019

Ready for another look @SteveSandersonMS @javiercn - I rebased this on top of my other PR so depending on when you look you might need to skip that commit.

@rynowak rynowak force-pushed the rynowak/uri-helper branch from da83ed2 to 6f82d07 Compare August 1, 2019 01:04
@rynowak
Copy link
Member Author

rynowak commented Aug 1, 2019

@SteveSandersonMS @javiercn Ping

@rynowak rynowak force-pushed the rynowak/uri-helper branch 2 times, most recently from bb97565 to 529494b Compare August 1, 2019 19:19
javiercn
javiercn approved these changes Aug 1, 2019
View reviewed changes
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.

Looks good!

I would have hope that we figured out something for all the shared initializations we need to do in this area, because I think we keep piling up those things in circuithost, but I recognize that that is orthogonal to this PR and we might get to do it if we have time.

The reason I mention this is because every time we have a concept like routing, auth, etc we end up having to add new public API for it.

@rynowak
Copy link
Member Author

rynowak commented Aug 1, 2019

I would have hope that we figured out something for all the shared initializations we need to do in this area, because I think we keep piling up those things in circuithost, but I recognize that that is orthogonal to this PR and we might get to do it if we have time.

Cool idea but I think it's really unlikely we'll be able to make this better. However, I think the concept count will be limited for 3-4 in this area.

Ryan Nowak and others added 8 commits August 1, 2019 15:14
Rename IUriHelper -> NavigationManager
4829479 
- Remove IUriHelper interface
- Rename to NavigationManager
- Remove all traces of old naming

There's no functional or design change in this commit - just removing
all traces of the old name. The next few iterations will try to improve
the design.
Minor API tweaks to NavigationManager
44cfb72 
Making Initialize protected causes problems because right now the
server-side code needs to deal with one of two different
implementations, hence an exchange type is used. I followed the same
pattern that was used for auth for symmetry but I have some *cool*
thoughts.

- We can remove this when we remove stateful prerendering
- I have another idea to banish this pattern to the land of wind and
ghosts

If this ends up sticking around longer than a week in the code, lets
discuss other ideas and try to improve the pattern.
Use hub method for server-side navigation
dfe1af5
Get rid of async local
5ce23ac
Add hub method test
d392405
Misc bikeshedding
8f058e3
@rynowak @campersau
Update src/Components/Server/src/Circuits/DefaultCircuitFactory.cs
759b09e 
Co-Authored-By: campersau <[email protected]>
@rynowak
PR feedback
2caaa0e
@rynowak rynowak force-pushed the rynowak/uri-helper branch from 529494b to 2caaa0e Compare August 1, 2019 22:17
@rynowak rynowak merged commit 9b888e9 into release/3.0 Aug 2, 2019
@ghost ghost deleted the rynowak/uri-helper branch August 2, 2019 00:11
@SteveSandersonMS SteveSandersonMS mentioned this pull request Aug 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveSandersonMS SteveSandersonMS SteveSandersonMS left review comments

@javiercn javiercn javiercn approved these changes

@ryanbrandenburg ryanbrandenburg Awaiting requested review from ryanbrandenburg

+3 more reviewers

@pranavkm pranavkm pranavkm left review comments

@campersau campersau campersau left review comments

@dougbu dougbu dougbu left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-blazor Includes: Blazor, Razor Components tell-mode Indicates a PR which is being merged during tell-mode

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@rynowak @SteveSandersonMS @pranavkm @campersau @dougbu @javiercn @Pilchie @mkArtakMSFT