Skip to content

Conversation

@oskarhane
Copy link
Member

@oskarhane oskarhane commented May 4, 2017

When trying to play a local guide: :play reco and it isn't found, an attempt to find it on the url browser.remote_content_hostname_whitelist + name is made.

browser.remote_content_hostname_whitelist is a server configuration list of hosts (or urls:s) that the browser can fetch remote content from.

The default value for browser.remote_content_hostname_whitelist is localhost,guides.neo4j.com, so the above play command would resolve and display the reco guide hosted on https://localhost/reco, then on http://localhost/reco, then on https://guides.neo4j.com/reco and finally on http://guides.neo4j.com/reco.
It stops after the first successful fetch.

The steps followed are:

  • Try locally. If found, display it
  • Try remotely, by appending the guide name to each of server config list of browser.remote_content_hostname_whitelist
  • Iterate over whitelist and if guide is found, display it and stop trying to fetch guide from the rest of the list
  • If error
    • show 'Not found' frame

Some updates to on how we use isomorphic-fetch are added as well, so errors are thrown when we hit a non 200 status from a request.

oskar4j 2017-05-04 at 11 10 23

@oskarhane oskarhane force-pushed the 3.0-resolve-to-remote-guides branch from 7e0a3c4 to 4a6c858 Compare May 8, 2017 14:01
@pe4cey pe4cey self-requested a review May 10, 2017 10:12
Copy link
Contributor

@pe4cey pe4cey left a comment

Choose a reason for hiding this comment

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

@oskarhane it seems that it only works when connected to the neo4j server.
To recreate: :server disconnect :play arrows

@oskarhane
Copy link
Member Author

@pe4cey Updated

@oskarhane
Copy link
Member Author

It'd be great to have your view on this feature and implementation @akollegger.

Should the config be moved to server config to have it centralized?
Suggestions for a better name on the configuration option would be appreciated. 🎈

@akollegger
Copy link
Member

Well, wdyt about two things:

  1. use the server whitelist as a "search path" for remote content
    2a. making the browser.* configuration served up by the server as static browser content
    2b. holding onto this PR until the server-side changes have been made for 2a

@oskarhane
Copy link
Member Author

I like the idea of using the whitelist as a search path @akollegger.
I'm not sure I understand what you mean by 2a.

Also, there might be the case where the browser isn't connected to a server (we might not even know the server url) and what should happen in that case? That's why I made it a client side config in the first place.
Maybe just a fallback til the guides url in that case?

@oskarhane oskarhane force-pushed the 3.0-resolve-to-remote-guides branch from 9ca56f8 to 8c3737a Compare May 16, 2017 20:28
@oskarhane oskarhane force-pushed the 3.0-resolve-to-remote-guides branch from 8c3737a to 95bc354 Compare May 16, 2017 20:43
@oskarhane
Copy link
Member Author

This is now ready for review again @pe4cey (and more comments if you have any @akollegger).
Since I've changed a lot I've updated the expected behavior in the PR:s description.

@oskarhane oskarhane changed the title Try to fetch remote guide if not found locally Try to fetch guide remotely if not found locally May 16, 2017
@oskarhane oskarhane force-pushed the 3.0-resolve-to-remote-guides branch from 95bc354 to e68bf2f Compare May 22, 2017 10:22
@oskarhane
Copy link
Member Author

Rebased @pe4cey

@oskarhane oskarhane force-pushed the 3.0-resolve-to-remote-guides branch from e68bf2f to 9dc51ed Compare May 24, 2017 07:56
@pe4cey pe4cey merged commit 8491c05 into neo4j:3.0 May 24, 2017
@oskarhane oskarhane deleted the 3.0-resolve-to-remote-guides branch May 26, 2017 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants