Skip to content

feat: Secure live updates #31

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

Merged
merged 3 commits into from
Oct 11, 2022
Merged

feat: Secure live updates #31

merged 3 commits into from
Oct 11, 2022

Conversation

Steven0351
Copy link
Contributor

Adds secure live updates for React Native. Also overhauled a bit of the code organization.

BREAKING: Makes previously non-async methods async. The module methods that were not async were essentially doing fire and forget making it impossible to synchronize on the user end. This was not much of an issue until adding secure live updates where the order in which those effects were executed were not in any guaranteed order. This technically isn't a source breaking change, but it is behavior changing.

and configuration based Portals.
feat(web): Make all exported functions awaitable.
chore(android): Reorganize code
Copy link

@giralte-ionic giralte-ionic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything looks good though I'm curious if the android config serializations should throw

null -> map.putNull(key)
else -> map.putString(key, value.toString())
}
} catch (_: JSONException) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this throw?

null -> array.pushNull()
else -> array.pushString(value.toString())
}
} catch (_: JSONException) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this throw?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception never should occur since we're accessing by index from 0 ..< length, but I also don't want to potentially throw an uncatchable exception and crash the application. Technically in Kotlin I could write this without being in the try/catch block since Kotlin doesn't have checked exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this method is for marshaling data to and from Portals to RN

@@ -3,7 +3,7 @@
// ReactNativePortals
//
// Created by Steven Sherry on 4/1/22.
// Copyright © 2022 Facebook. All rights reserved.
// Copyright © 2022 Ionic. All rights reserved.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops lol :)

val configJson = try {
JSONObject(config)
} catch (e: JSONException) {
throw Error("Portals config data is malformed. Aborting.", e)
Copy link
Contributor Author

@Steven0351 Steven0351 Oct 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giralte-ionic we throw here if the config is not serializable

usesSecureLiveUpdates = true
}

val portalJsonArray = configJson.getJSONArray("portals")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giralte-ionic this throws here if the portals key is not available

val portalJsonArray = configJson.getJSONArray("portals")

for (index in 0 until portalJsonArray.length()) {
val portalJson = portalJsonArray.getJSONObject(index)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giralte-ionic this throws here if the index is not accessible

@Steven0351 Steven0351 merged commit 2b0e5b5 into main Oct 11, 2022
@Steven0351 Steven0351 deleted the secure-live-updates branch October 11, 2022 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants