feat(GUI): add webview api version parameter #1558
Conversation
| * @private | ||
| * @type {String} | ||
| */ | ||
| const API_VERSION_PARAM = 'api-version'; |
There was a problem hiding this comment.
Can we add a big comment in the @description section stating that changing this will break compatibility with the HTML banner and therefore we should only change it if absolutely necessary?
There was a problem hiding this comment.
I guess you could also explain that the HTML banner can use this version-number to determine which features the safe-webview supports (independent of the Etcher version-number).
(perhaps it's just me, but 'API' feels like the wrong term for this)
There was a problem hiding this comment.
Ah, you addressed it right below, sorry!
lib/gui/components/safe-webview.js
Outdated
| * @private | ||
| * @type {String} | ||
| */ | ||
| const API_VERSION = 1; |
There was a problem hiding this comment.
Should we start off at 2, and treat API_VERSION 1 as the version from before the api-version key was added?
There was a problem hiding this comment.
@jviotti mentioned that we should just treat no given API_VERSION as such, which we can call the the "zeroth" API version I guess.
lib/gui/components/safe-webview.js
Outdated
| @@ -69,6 +85,7 @@ class SafeWebview extends react.PureComponent { | |||
|
|
|||
| // We set the version GET parameter here. | |||
| url.searchParams.set(VERSION_PARAM, packageJSON.version); | |||
There was a problem hiding this comment.
Perhaps the VERSION_PARAM constant should now be renamed to ETCHER_VERSION_PARAM ?
And I guess the comment above could be changed to // We set the version GET parameters here. ;)
| * @private | ||
| * @type {String} | ||
| */ | ||
| const API_VERSION_PARAM = 'api-version'; |
There was a problem hiding this comment.
I guess you could also explain that the HTML banner can use this version-number to determine which features the safe-webview supports (independent of the Etcher version-number).
(perhaps it's just me, but 'API' feels like the wrong term for this)
Shou
force-pushed
the
webview-api-version
branch
from
32082e7
to
53c00fa
Compare
|
Once the tests pass we should be good to go. |
lib/gui/components/safe-webview.js
Outdated
| @@ -46,6 +46,28 @@ const VERSION_PARAM = 'etcher-version'; | |||
| const ELECTRON_SESSION = 'persist:success-banner'; | |||
|
|
|||
| /** | |||
| * @summary API version search parameter key | |||
There was a problem hiding this comment.
Minor nit, but could we hyphenate this as search-parameter ? To stop people thinking that "version search" is a thing ;)
And could we update the @summary for ETCHER_VERSION_PARAM so that the two read similarly? (and given their similarity, perhaps ETCHER_VERSION_PARAM and API_VERSION_PARAM should be moved next to each other in the source-code?)
|
Looks like the Travis builds have been broken by #1553 ? |
|
@Shou Hopefully if you rebase this to master, the Travis tests should work now? :) |
|
@lurch doesn't look that way, unfortunately. |
|
The TravisCI logfiles (even in 'raw' mode) are still dying in a seemingly random place 😢 |
|
@lurch didn't get it last Friday, but have just kicked off https://travis-ci.org/resin-io/etcher/builds/249631499, with silenced output – which will hopefully get us some insight as to what's happening there |
|
@Shou Could you try rebasing again? Looks like Jonas and Juanchi (the J-brothers?) have fixed it :) |
We add the API version sent to the banner via a GET search parameter, allowing for more intricate control over what data needs or can be sent. Changelog-Entry: Add Webview API version parameter.
|
Awesome, it's all pristine green now. |
We add the API version sent to the banner via a GET search parameter,
allowing for more intricate control over what data needs or can be sent.
Changelog-Entry: Add Webview API version parameter.