-
Notifications
You must be signed in to change notification settings - Fork 63
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(android): fix JS error when RN version is a github URL #38
Changes from 2 commits
a40db28
1d558c6
5a8938d
9628be0
6b68286
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
const path = require("path"); | ||
const fs = require("mz/fs"); | ||
const compareVersions = require("compare-versions"); | ||
const validate = require("compare-versions"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dlowder-salesforce There looks to be a duplicate import here. This package is being imported above. |
||
|
||
const pkg = require(path.join(process.cwd(), "package.json")); | ||
|
||
|
@@ -17,9 +18,15 @@ function readFile(file, readDirPath) { | |
function parseFile(fileData, { templateName, packageName, app, rnVersion }) { | ||
let kotlinPackage; | ||
let javaPackage; | ||
var version = rnVersion; | ||
try { | ||
validate(rnVersion); | ||
} catch (e) { | ||
version = null; | ||
} | ||
|
||
// TODO: figure out a better way to handle one off breaking changes | ||
if (rnVersion && compareVersions(rnVersion, "0.47.2") < 0) { | ||
if (version && compareVersions(version, "0.47.2") < 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dlowder-salesforce I am realizing that your version check just validates a github repo as a valid source, however you don't get the actual version from it. How will There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually a github repo string will fail the validation check, so There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see...so I am actually wondering if this PR will cover this issue too and just needs a test https://github.com/peggyrayzis/react-native-create-bridge/pull/37/files There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that looks like a similar fix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dlowder-salesforce Since there are redundancies Im going to add add you test into the other PR and make sure it is still working as expected. I'll add you as a reviewer when it's done. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jarretmoses sounds good, I'll close this PR then |
||
kotlinPackage = ` | ||
override fun createJSModules(): List<Class<out JavaScriptModule>> { | ||
return emptyList() | ||
|
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.
is this necessary?
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.
It was needed in my environment.