Use external Electron to Chromium library #144
Conversation
| @@ -2,7 +2,7 @@ | |||
| "presets": [ | |||
| ["../../../../lib", { | |||
| "targets": { | |||
| "electron": 1.5 | |||
| "electron": "1.4" | |||
There was a problem hiding this comment.
Maybe avoid strings unless we will maintain it for all targets?
There was a problem hiding this comment.
will do. I did this before updating my library to also handle numbers, so it can be reverted.
| @@ -141,7 +121,10 @@ export const getTargets = (targets = {}) => { | |||
|
|
|||
| // Rewrite Electron versions to their Chrome equivalents | |||
| if (targetOps.electron) { | |||
| targetOps.chrome = electronVersionToChromeVersion(targetOps.electron); | |||
| targetOps.chrome = parseInt(electronToChromium(targetOps.electron), 10); | |||
There was a problem hiding this comment.
If we have for ex:
"targets": {
"chrome": 50,
"electron": 1.3 // 52
}looks like it will just rewrite chrome version with higher. So the result will be chrome: 52, but not 50.
There was a problem hiding this comment.
I'll do a Math.min()
|
I didn't want to change the way the behaviour worked with regards to how it handles the electron -> chrome conversion so that works the same as it currently does: set the targetOps.chrome and delete targetOps.electron. Logging it as electron and keeping the chrome conversion on a lower level is better suited for a different PR, I think. |
|
Just pushed the requested changes! |
| throw new Error(`Electron version ${targetOps.electron} is either too old or too new`); | ||
| } | ||
|
|
||
| if (targetOps.chrome) { |
There was a problem hiding this comment.
This might be a good test case?
There was a problem hiding this comment.
you're right! will add it tonight.
|
@yavorsky I think all the requested changes are put in! ...And now it can not be merged back to master :( I'll see how much trouble it is to rebase. |
… version and update expected output
Kilian
force-pushed
the
electron-to-chromium
branch
from
33363b8
to
b45923b
Compare
|
Rebased on latest master and all green, should be good to go now. |
| @@ -124,7 +103,18 @@ export const getTargets = (targets = {}) => { | |||
|
|
|||
| // Rewrite Electron versions to their Chrome equivalents | |||
| if (targetOps.electron) { | |||
| targetOps.chrome = electronVersionToChromeVersion(targetOps.electron); | |||
| const electronChromeVersion = parseInt(electronToChromium(targetOps.electron), 10); | |||
There was a problem hiding this comment.
I don't think parseInt(a, 10); is necessary anymore in ES5? Can remove later
|
Thanks for the library + PR! |
Per the suggestion in ticket #142 this pull request switches out the local list of electron-to-chromium mappings for a more complete external library.
I had to update a number of tests, because Electron 1.5 is not out yet and it's not guaranteed it will use the assumed chrome version. Additionally, the old data file linked electron 1.0 to chrome 50 but that should have been 49, which I updated in the tests as well.
I updated the external library to allow both string and numerical input, but it always returns a string (because it can return full chromium versions too, which require it) so I
parseInt()that in the script. I hope that's ok.