Here is some feedback for @joshuef from @Krishna and the rest of the MaidSafe team
General
1: It would be useful to make the “New tab” page more relevant to SAFE Network users. For example:
- replacing the links in the top right corner (“Feedback & Discussion”, “Report Bug” and “Help”) with links specific to the SAFE Network and SAFE Browser (e.g. https://safenetforum.org (you could link to a specific topic about SAFE Browser) and Issues · joshuef/beaker · GitHub). In SAFE-only mode, maybe http:// links should open in an external browser?
- replacing the default favorites by safe:// websites (e.g.
safe://diy.yvette/
) - removing the “Dat Archives” link and the “Share Files” button
2: It would be advantageous for the browser to check whether SAFE Launcher is running when it starts.
3: It would be great if the browser could register a protocol handler with the operating system, so that safe://
can be opened by default with SAFE Browser
Application packaging
1: Please add the new SAFE Network logo, SVG version available here: Providing Privacy, Security and Freedom | MaidSafe.
Comments from Francis:
The SAFE Network logo has been added in the last update.
In macOS it doesn’t look great because it shows up as a square (because of the white background). I’m thinking it could look good to have the SAFE Network logo in white and contained within a blue circle.
the current version is probably fine for now. someone who is good at graphics design could submit a better version of the application icon. in the browser “New tab” page it looks fine.
2: The packaged application should follow the naming convention (e.g. safe-browser-v0.3.0-win-x64
)
Right now the names look like this:
- SAFE.Beaker.Browser-0.3.0-win.zip
- SAFE.Beaker.Browser-0.3.0.dmg
- safer-beaker-browser-0.3.0.zip
The naming of the packages should be different and follow the convention from SAFE Launcher, SAFE Demo App, etc. And the application name could be made shorter (e.g. “SAFE Browser” instead of “SAFE Beaker Browser”).
- safe-browser-v0.3.0-win-x64.zip
- safe-browser-v0.3.0-osx-x64.dmg
- safe-browser-v0.3.0-linux-x64.zip
Also, when unzipping a file such as safe-browser-v0.3.0-win-x64.zip
, the name of the folder should be safe-browser-v0.3.0-win-x64
instead of win-unpacked
.
API
1: Please consider parsing and returning the responses from the APIs. At present it returns the entire response object of the fetch-isomorphic
module
For example, the response can be:
{
Status: 200,
Headers: {
},
Body: JSON or Buffer or Uint8Array
}
Comments from Krishna:
This is a mere example. Wanted to ensure that we can have a simple and clear response object.
This has to be designed with some thought put in. We might not even need a http response like structure. Can simply be the result itself.
If the response is JSON, then just the body of the response should do. ( resolve(obj) )
For operations like PUT and POST where there is no result returned, there won’t be any return value. ( resolve() )
Since the library is promised based, this approach can be useful.
If we are projecting the injected APIs as ajax equivalent then returning a simple response object is fine. But if the APIs are designed to be used as helper functions then returning the result object directly is preferred. This is just my opinion
Sometimes there is both a header and a body (e.g. https://api.safedev.org/nfs/file/get-file.html)
Yes, this is a case in which the binary data is streamed.
First the headers are sent and only after that the data is written from server.
If we are supporting streaming of data via callbacks, then the first data received in the callback can be the headers (metadata) of the file and then the actual contents can be sent via the callback.
This is just one approach.
2: Please consider improving the errors returned from the APIs and/or make them consistent so that the same response format can be used. The only difference will be to invoke Promise.reject
and pass the response. This will improve the information that can be derived from the error messages at run time, since the status code and text are returned as a part of message string.
3: Please consider making the tokenKey in authorise function in safe-js mandatory for apps to pass, instead of setting a default value.
Comments from Krishna:
There is a bigger issue with this one IMO.
Consider
App A
on one tab is using a TOKEN = ‘auth_token’ to store and retrieve its auth token using the present local storage option provided in safe-js.
Another application can also use a same token name. This will also result in overriding the token and also to easily extract other applications authorisation token.
If
App A
chooses to randomly generate a token key, then for when the same application is opened in another tab a new authorisation request will have to be sent. This seems like a workaround but it would become an annoying UX
Suggestion from Francis:
maybe the name of the token could be generated by
safe-js
based on the value ofwindow.location.host
? so apps wouldn’t have the option to use another name, the token name would be generated by the browser based on the subdomain and the domain name of the current page (e.g. safe://blog.example/post.html would generate a token key called “blog.example”). and retrieving the token would also be based onwindow.location.host
(so you can’t retrieve a token from another site). do you think that would make sense?
Comment from Krishna:
I think so. But I am not sure, whether safe-js will be able to read the tab from which the request is originating. Can check with Josh
4: Streaming APIs like file read should be sent via callback instead of waiting for the entire content to download. It would be great if you could make this change.
Comment from Krishna:
Not able to think of a better alternative to callbacks. If there is a better alternative then it would make it even better.
5: Have not tested audio/video streaming using html5 controls. Based on the code, in safe_protocol registration it is only translating the URL scheme. In case of default video controls of HTML5, they use range headers for streaming.