-
-
Notifications
You must be signed in to change notification settings - Fork 40
refactor: fetch v8Versions map from external url #242
Conversation
a3f8314 to
64d9ff0
Compare
48999fe to
5079bd9
Compare
5079bd9 to
189a4c2
Compare
bin/ns-bundle
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do, thanks for noticing that!
bin/ns-bundle
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 toString is recommended in some style guides
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}`);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can optionally shorten this syntax here:
const runtimeRange = Object.keys(v8VersionsMap).find(range =>semver.satisfies(runtimeVersion, range));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@petekanev
petekanev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and clean.
Uh oh!
There was an error while loading. Please reload this page.
Https request to the
v8-versions.jsonfile from the android-runtime repo is used to determine the used v8 version.