refactor: fetch v8Versions map from external url #242
Conversation
sis0k0
force-pushed
the
vlaeva/fetch-v8-versions
branch
from
a3f8314 to
64d9ff0
Compare
sis0k0
force-pushed
the
vlaeva/fetch-v8-versions
branch
3 times, most recently
from
48999fe to
5079bd9
Compare
sis0k0
force-pushed
the
vlaeva/fetch-v8-versions
branch
from
5079bd9 to
189a4c2
Compare
sis0k0
changed the title
bin/ns-bundle
Outdated
| const platform = options.platform; | ||
| let commands = [ | ||
| () => runTns("prepare", platform) | ||
| // () => runTns("prepare", platform) |
There was a problem hiding this comment.
We do, thanks for noticing that!
bin/ns-bundle
Outdated
| const childProcess = spawn("tns", ["--version"], { shell: true }); | ||
|
|
||
| childProcess.stdout.on("data", resolve); | ||
| childProcess.stdout.on("data", version => resolve(String(version))); |
There was a problem hiding this comment.
I think I'd prefer version.toString() here to String(version).
The latter looks a bit odd, just a personal taste though, although there are some minor differences between the two.
There was a problem hiding this comment.
👍 toString is recommended in some style guides
| "generate-android-snapshot": "./bin/generate-android-snapshot" | ||
| }, | ||
| "dependencies": { | ||
| "semver": "^5.4.1", |
There was a problem hiding this comment.
An exact version would be better IMO - ^5.4.1 could cause problems in case semver releases 5.5.0 which has breaking changes (which shouldn't happen but can)
There was a problem hiding this comment.
I believe the semver package will follow the semver versioning. Also if there are some changes that they need to introduce because of new node/npm versions, they'll probably do it with minor releses and not patches.
| } | ||
| }); | ||
| }).catch(error => { | ||
| throw new Error("Cannot find suitable v8 version!"); |
There was a problem hiding this comment.
Do you think it would be sensible to add information about the actual error that occurred here, like for example:
throw new Error(`Cannot find suitable v8 version! Original error: ${error.message || error}`);
| const runtimeVersion = this.getAndroidRuntimeVersion().replace(/-.*/, ""); | ||
|
|
||
| const runtimeRange = Object.keys(v8VersionsMap).find(range => { | ||
| return semver.satisfies(runtimeVersion, range) |
There was a problem hiding this comment.
You can optionally shorten this syntax here:
const runtimeRange = Object.keys(v8VersionsMap).find(range =>semver.satisfies(runtimeVersion, range));
Https request to the
v8-versions.jsonfile from the android-runtime repo is used to determine the used v8 version.