Open ExtensionApps at extension's landing page #113
Conversation
| self.serverapp.open_browser = True | ||
| # Hijack the server's browser-open file to land on | ||
| # the extensions home page. | ||
| self.serverapp._write_browser_open_file = self._write_browser_open_file |
There was a problem hiding this comment.
This seems like the wrong way to accomplish this.
There was a problem hiding this comment.
Hm, maybe. Let me explain what my reason:
I found two ways for an extension to reroute the server landing page.
- Change the server's
default_urland have it land on the extension. There were two challenges with this approach:- The server's
default_urlneeds to be set to<extension>.default_urlbefore theserver.initialize()is called, otherwise we have to change a bunch of downstream trails ofdefault_urlin serverapp. The issue is that<extension>.default_urldoesn't get set until after server is initialized. - The server's
default_urltruly is a different trait than the extension'sdefault_url. In my mind, they could/should point to two different urls (though, I don't have an example of when someone might want to do this right now).
- The server's
- Change the server's browser open file. I chose this approach because:
- The server doesn't open a browser window by default, anyways. In fact, a browser window doesn't really make sense without an
ExtensionApp. - This file is only created if the server was started by an extension. In that case, I think it makes sense that the server's browser file always opens to the extension's homepage.
- The
ServerApp.default_urlandExtensionApp.default_urlcan be pointing to different URLs (maybe a plus?).
- The server doesn't open a browser window by default, anyways. In fact, a browser window doesn't really make sense without an
There was a problem hiding this comment.
When I wrote "wrong way', I meant to overwrite a protected method on the app. This is basically monkey-patching, and it means that there isn't really a clear API boundary between the two objects.
| path = url_path_join(self.base_url, self.default_url) | ||
| # Build query string. | ||
| if self.token: | ||
| token = urllib.parse.urlencode({'token': token}) |
There was a problem hiding this comment.
This should probably raise an error if token is None or default to '...'
There was a problem hiding this comment.
What if token is actually None, though?
Before this PR, token=None returned a url without a token. This PR maintains that pattern. Do we always want to require a token and change previous behavior in the PR?
There was a problem hiding this comment.
Oh wait, I missed your actual point... 🤦♂
Yes, this will return token=None in the url. Good catch!
There was a problem hiding this comment.
The conditional should be if token, not if self.token. That was a typo.
ie.
if token:
token = urllib.parse.urlencode({'token': token})There are cases where we don't want the token, for example in self.connection_url.
| """Build a url for the application. | ||
| """ | ||
| if not ip: | ||
| ip = self.ip |
There was a problem hiding this comment.
This should probably raise if not self.ip, or default to localhost.
| """URL to display to the user.""" | ||
| # Get url from server. | ||
| url = url_path_join(self.serverapp.base_url, self.default_url) | ||
| return self.serverapp.get_url(self.serverapp.ip, url) |
There was a problem hiding this comment.
Won't this fail if self.token is truthy?
There was a problem hiding this comment.
In #115, this returns a url without a token. That's what we want here. The display_url property adds a token to custom_display_url.
|
Fair enough... I'll give others more time to review before merging in the future :-) |
* move deployment pieces here * ignore deployment in check manifest * add deployment directory to ignored check-manifest list * blend publishing step * remove blend * bad yaml
Three things have changed:
ExtensionApppatches the server's browser-open file and open at the extension'sdefault_url. This logic only applies when the extension is launched directly (not loaded using theServerApp.load_server_extensions()method).ExtensionApppatches the server'sdisplay_url(what's shown in the terminal when you launch) by overwriting thecustom_display_urltrait of the server.URLS in the ServerApp are constructed using
urllib. Provides a cleaner way to construct/deconstruct urls.