-
Notifications
You must be signed in to change notification settings - Fork 0
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
EMBR-4636 calculate react native bundle id #72
Changes from 2 commits
1707dce
19fe331
d05c4e9
5248660
f6f2275
cf514f5
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 |
---|---|---|
@@ -0,0 +1,67 @@ | ||
import Foundation | ||
import EmbraceIO | ||
import CryptoKit | ||
|
||
private struct LastBundleComputation: Codable { | ||
private static let STORAGE_KEY = "EMBRACE_LAST_BUNDLE_COMPUTATION" | ||
var path: String | ||
var computedAt: Date? | ||
var id: String | ||
|
||
func store () { | ||
if let encoded = try? JSONEncoder().encode(self) { | ||
UserDefaults.standard.set(encoded, forKey: LastBundleComputation.STORAGE_KEY) | ||
} | ||
} | ||
|
||
static func fromStorage() -> LastBundleComputation { | ||
if let storedData = UserDefaults.standard.object(forKey: LastBundleComputation.STORAGE_KEY) as? Data, | ||
let loaded = try? JSONDecoder().decode(LastBundleComputation.self, from: storedData) { | ||
return loaded | ||
} | ||
|
||
return LastBundleComputation(path: "", computedAt: nil, id: "") | ||
} | ||
} | ||
|
||
enum ComputeBundleIDErrors: Error { | ||
case emptyPath | ||
} | ||
|
||
struct BundleID { | ||
var id: String | ||
var cached: Bool | ||
} | ||
|
||
func computeBundleID(path: String) throws -> BundleID { | ||
if path.isEmpty { | ||
throw ComputeBundleIDErrors.emptyPath | ||
} | ||
|
||
var last = LastBundleComputation.fromStorage() | ||
|
||
// Check if we can re-use our last computed bundle ID, this is true only if the path hasn't changed and | ||
// the file contents at that path haven't been modified since the last time we computed the ID | ||
// TODO in ios 5.x and Android implementation we are just returning last.id in this case without checking modified at...is that correct? | ||
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. @ArielDemarco @TheMutsi this logic feels correct to me, e.g. only re-use the cache if the path is the same and the contents haven't been updated but it does deviate from the previous implementations so wanted to double-check 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. LGTM; I'd wait for @TheMutsi to see if there's nothing we're not considering 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. @ArielDemarco can we move it to DispatchQueue bg? I think it could be very heavy 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 think that makes sense, I moved the calls to compute ID to background queues |
||
if path == last.path { | ||
let attributes = try? FileManager.default.attributesOfItem(atPath: path) | ||
if let lastComputed = last.computedAt, let modifiedAt = attributes?[FileAttributeKey.modificationDate] as? Date, | ||
modifiedAt <= lastComputed { | ||
return BundleID(id: last.id, cached: true) | ||
} | ||
} | ||
|
||
let fileContents = try String(contentsOf: URL(fileURLWithPath: path)) | ||
|
||
// https://stackoverflow.com/a/56578995 | ||
let bundleID = Insecure.MD5.hash(data: Data(fileContents.utf8)).map { | ||
jpmunz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
String(format: "%02hhx", $0) | ||
}.joined() | ||
|
||
last.computedAt = Date() | ||
last.path = path | ||
last.id = bundleID | ||
last.store() | ||
|
||
return BundleID(id: last.id, cached: false) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ import CodePush | |
private let JAVASCRIPT_PATCH_NUMBER_RESOURCE_KEY = "javascript_patch_number" | ||
private let HOSTED_PLATFORM_VERSION_RESOURCE_KEY = "hosted_platform_version" | ||
private let HOSTED_SDK_VERSION_RESOURCE_KEY = "hosted_sdk_version" | ||
private let REACT_NATIVE_BUNDLE_ID_RESOURCE_KEY = "react_native_bundle_id" | ||
|
||
// Keys defined in packages/spans/interfaces/ISpans.ts | ||
private let EVENT_NAME_KEY = "name" | ||
|
@@ -43,10 +44,9 @@ class EmbraceManager: NSObject { | |
@objc(setJavaScriptBundlePath:resolver:rejecter:) | ||
func setJavaScriptBundlePath(_ path: String, resolver resolve: @escaping RCTPromiseResolveBlock, rejecter reject: @escaping RCTPromiseRejectBlock) { | ||
do { | ||
|
||
// TODO, use path to compute a hash of the assets and add as a 'react_native_bundle_id' key to the resource | ||
// try Embrace.client?.metadata.addResource(key: EmbraceKeys.javaScriptBundleURL.rawValue, value: path, lifespan: .process) | ||
resolve(true) | ||
let bundleID = try computeBundleID(path: path) | ||
try Embrace.client?.metadata.addResource(key: REACT_NATIVE_BUNDLE_ID_RESOURCE_KEY, value: bundleID.id, lifespan: .process) | ||
resolve(true) | ||
} catch let error { | ||
reject("SET_JS_BUNDLE_PATH_ERROR", "Error setting JavaScript bundle path", error) | ||
} | ||
|
@@ -204,23 +204,22 @@ class EmbraceManager: NSObject { | |
_ resolve: @escaping RCTPromiseResolveBlock, | ||
rejecter reject: @escaping RCTPromiseRejectBlock | ||
) { | ||
// do { | ||
// #if canImport(CodePush) | ||
// guard let url = try CodePush.bundleURL() else { | ||
// reject("GET_CODEPUSH_BUNDLE_URL", "Error getting Codepush Bundle URL", nil) | ||
// return | ||
// } | ||
|
||
// // TODO, use path to compute a hash of the assets and add as a 'react_native_bundle_id' key to the resource | ||
// try Embrace.client?.metadata.addResource(key: EmbraceKeys.javaScriptBundleURL.rawValue, value: url.path, .process) | ||
// resolve(true) | ||
|
||
// #else | ||
// resolve(false) | ||
// #endif | ||
// } catch let error { | ||
// reject("CHECK_AND_SET_CODEPUSH_BUNDLE_URL", "Error setting Codepush Bundle URL", error) | ||
// } | ||
do { | ||
#if canImport(CodePush) | ||
guard let url = try CodePush.bundleURL() else { | ||
reject("GET_CODEPUSH_BUNDLE_URL", "Error getting Codepush Bundle URL", nil) | ||
return | ||
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 wonder if it's better not maintain "codepush" in a specific method, and just let the user set the javascript url if they are using codepush. @jpmunz what do you think? we can just remove this function and update the doc so we dont care if Codepush changes its name or method in the future 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 think I'd keep it as is for the purposes of the iOS 6.x migration but I'd like to revisit along with calculating the bundle ID altogether to see if we can reduce code here, noted in https://www.notion.so/embraceio/Rethink-maintaining-support-for-OTA-updates-efe0b4b089ba487d9ab6678b7f7ca491?pvs=4 |
||
} | ||
|
||
let bundleID = try computeBundleID(path: path) | ||
try Embrace.client?.metadata.addResource(key: REACT_NATIVE_BUNDLE_ID_RESOURCE_KEY, value: bundleID.id, lifespan: .process) | ||
resolve(true) | ||
#else | ||
resolve(false) | ||
#endif | ||
} catch let error { | ||
reject("CHECK_AND_SET_CODEPUSH_BUNDLE_URL", "Error setting Codepush Bundle URL", error) | ||
} | ||
resolve(false) | ||
} | ||
|
||
|
@@ -387,7 +386,6 @@ class EmbraceManager: NSObject { | |
} | ||
} | ||
|
||
|
||
@objc(logNetworkRequest:httpMethod:startInMillis:endInMillis:bytesSent:bytesReceived:statusCode:resolver:rejecter:) | ||
func logNetworkRequest( | ||
_ url: String, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -694,13 +694,13 @@ class EmbraceSpansTests: XCTestCase { | |
let exportedSpans = try await getExportedSpans() | ||
XCTAssertEqual(exportedSpans.count, 2) | ||
} | ||
|
||
func testLogNetworkRequest() async throws { | ||
module.logNetworkRequest("https://otest.com/v1/products", httpMethod: "get", startInMillis: 1723221815889, endInMillis: 1723221815891, bytesSent: 1000, bytesReceived: 2000, statusCode: 200, resolver: promise.resolve, rejecter: promise.reject) | ||
|
||
XCTAssertEqual(promise.resolveCalls.count, 1) | ||
XCTAssertTrue((promise.resolveCalls[0] as? Bool)!) | ||
|
||
module.logNetworkRequest("https://otest.com/", httpMethod: "POST", startInMillis: 1723221815889, endInMillis: 1723221815891, bytesSent: -1, bytesReceived: -2, statusCode: -1, resolver: promise.resolve, rejecter: promise.reject) | ||
|
||
module.logNetworkRequest("https://otest.com/v2/error", httpMethod: "POST", startInMillis: 1723221815889, endInMillis: 1723221815891, bytesSent: -1, bytesReceived: -2, statusCode: 500, resolver: promise.resolve, rejecter: promise.reject) | ||
|
@@ -716,7 +716,7 @@ class EmbraceSpansTests: XCTestCase { | |
XCTAssertEqual(exportedSpans[0].attributes["http.response.body.size"]!.description, "2000") | ||
XCTAssertEqual(exportedSpans[0].attributes["http.request.body.size"]!.description, "1000") | ||
XCTAssertEqual(exportedSpans[0].attributes["http.response.status_code"]!.description, "200") | ||
|
||
XCTAssertEqual(exportedSpans[1].name, "emb-POST") | ||
XCTAssertEqual(exportedSpans[1].startTime, Date(timeIntervalSince1970: 1723221815.889)) | ||
XCTAssertEqual(exportedSpans[1].endTime, Date(timeIntervalSince1970: 1723221815.891)) | ||
|
@@ -726,22 +726,22 @@ class EmbraceSpansTests: XCTestCase { | |
XCTAssertNil(exportedSpans[1].attributes["http.response.body.size"]) | ||
XCTAssertNil(exportedSpans[1].attributes["http.request.body.size"]) | ||
XCTAssertNil(exportedSpans[1].attributes["http.response.status_code"]) | ||
|
||
XCTAssertEqual(exportedSpans[2].name, "emb-POST /v2/error") | ||
XCTAssertEqual(exportedSpans[2].startTime, Date(timeIntervalSince1970: 1723221815.889)) | ||
XCTAssertEqual(exportedSpans[2].endTime, Date(timeIntervalSince1970: 1723221815.891)) | ||
XCTAssertEqual(exportedSpans[2].attributes["url.full"]!.description, "https://otest.com/v2/error") | ||
XCTAssertEqual(exportedSpans[2].attributes["http.response.status_code"]!.description, "500") | ||
// NOTE: is this correct? Android is setting `status` as `Error` when the status code represents an error code (like in this case) | ||
XCTAssertEqual(exportedSpans[2].status, Status.unset); | ||
XCTAssertEqual(exportedSpans[2].status, Status.unset) | ||
} | ||
|
||
func testLogNetworkClientError() async throws { | ||
module.logNetworkClientError("https://otest.com/v1/products", httpMethod: "get", startInMillis: 1723221815889, endInMillis: 1723221815891, errorType: "custom error", errorMessage: "this is my error", resolver: promise.resolve, rejecter: promise.reject) | ||
|
||
XCTAssertEqual(promise.resolveCalls.count, 1) | ||
XCTAssertTrue((promise.resolveCalls[0] as? Bool)!) | ||
|
||
let exportedSpans = try await getExportedSpans() | ||
|
||
XCTAssertEqual(exportedSpans[0].name, "emb-GET /v1/products") | ||
|
@@ -751,7 +751,7 @@ class EmbraceSpansTests: XCTestCase { | |
XCTAssertEqual(exportedSpans[0].attributes["error.message"]!.description, "this is my error") | ||
XCTAssertEqual(exportedSpans[0].attributes["error.type"]!.description, "custom error") | ||
} | ||
|
||
func testStartEndView() async throws { | ||
module.startView("my-view", resolver: promise.resolve, rejecter: promise.reject) | ||
XCTAssertEqual(promise.resolveCalls.count, 1) | ||
|
@@ -799,3 +799,70 @@ class EmbraceSpansSDKNotStartedTests: XCTestCase { | |
} | ||
} | ||
|
||
class ComputeBundleIDTests: XCTestCase { | ||
// https://nshipster.com/temporary-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. Probably many of this tests under the hood, are using override func tearDownWithError() throws {
UserDefaults.standard.removePersistentDomain(forName: "EmbraceReactNative")
} 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. added |
||
func writeTempFile(contents: String, url: URL? = nil) throws -> URL { | ||
var to = url | ||
if to == nil { | ||
let tmpDir = try FileManager.default.url(for: .itemReplacementDirectory, | ||
in: .userDomainMask, | ||
appropriateFor: URL(fileURLWithPath: "sample_bundle.js"), | ||
create: true) | ||
|
||
to = tmpDir.appendingPathComponent(ProcessInfo().globallyUniqueString) | ||
} | ||
|
||
try contents.write(to: to!, atomically: true, encoding: String.Encoding.utf8) | ||
|
||
return to! | ||
} | ||
|
||
func testEmptyPath() { | ||
XCTAssertThrowsError(try computeBundleID(path: "")) { error in | ||
XCTAssertEqual(error as? ComputeBundleIDErrors, ComputeBundleIDErrors.emptyPath) | ||
} | ||
} | ||
|
||
func testNothingCached() { | ||
let fileURL = try? writeTempFile(contents: "console.log('my js bundle');") | ||
let bundleID = try? computeBundleID(path: fileURL!.path()) | ||
XCTAssertEqual(bundleID?.id, "29da484ad2a259e9de64a991cfec7f10") | ||
XCTAssertFalse(bundleID!.cached) | ||
} | ||
|
||
func testDifferentPath() { | ||
let fileURL = try? writeTempFile(contents: "console.log('my js bundle');") | ||
var bundleID = try? computeBundleID(path: fileURL!.path()) | ||
XCTAssertEqual(bundleID?.id, "29da484ad2a259e9de64a991cfec7f10") | ||
XCTAssertFalse(bundleID!.cached) | ||
|
||
let otherURL = try? writeTempFile(contents: "console.log('my other bundle');") | ||
bundleID = try? computeBundleID(path: otherURL!.path()) | ||
XCTAssertEqual(bundleID?.id, "9a7ef24cdaf5cdb927eab12cb0bfd30d") | ||
XCTAssertFalse(bundleID!.cached) | ||
} | ||
|
||
func testSamePathNotModified() { | ||
let fileURL = try? writeTempFile(contents: "console.log('my js bundle');") | ||
var bundleID = try? computeBundleID(path: fileURL!.path()) | ||
XCTAssertEqual(bundleID?.id, "29da484ad2a259e9de64a991cfec7f10") | ||
XCTAssertFalse(bundleID!.cached) | ||
|
||
bundleID = try? computeBundleID(path: fileURL!.path()) | ||
XCTAssertEqual(bundleID?.id, "29da484ad2a259e9de64a991cfec7f10") | ||
XCTAssertTrue(bundleID!.cached) | ||
} | ||
|
||
func testSamePathModified() { | ||
let fileURL = try? writeTempFile(contents: "console.log('my js bundle');") | ||
var bundleID = try? computeBundleID(path: fileURL!.path()) | ||
XCTAssertEqual(bundleID?.id, "29da484ad2a259e9de64a991cfec7f10") | ||
XCTAssertFalse(bundleID!.cached) | ||
|
||
let sameURL = try? writeTempFile(contents: "console.log('my other bundle');", url: fileURL) | ||
XCTAssertEqual(fileURL, sameURL) | ||
bundleID = try? computeBundleID(path: sameURL!.path()) | ||
XCTAssertEqual(bundleID?.id, "9a7ef24cdaf5cdb927eab12cb0bfd30d") | ||
XCTAssertFalse(bundleID!.cached) | ||
} | ||
} |
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'd inject an "own"
UserDefaults
to prevent any problem in the future (like somebody migrating to another suite, or things like that).The only bad thing about injecting it (and having an instance variable with an own
userDefaults
) is that you'll have to implementCodable
manually.So, i'd suggest something in the middle, like providing a utility to create an "Embrace" UserDefaults, like this:
And then you can replace
store
andfromStorage
using that utility: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.
makes sense, added