EMBR-4636 calculate react native bundle id #72
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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. @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. LGTM; I'd wait for @TheMutsi to see if there's nothing we're not considering There was a problem hiding this comment. @ArielDemarco can we move it to DispatchQueue bg? I think it could be very heavy There was a problem hiding this comment. 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) | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 { | ||
| 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 | ||
|
There was a problem hiding this comment. 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. 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) | ||
| } | ||
|
|
||
|
|
@@ -381,7 +380,6 @@ class EmbraceManager: NSObject { | |
| } | ||
| } | ||
|
|
||
| @objc(logNetworkRequest:httpMethod:startInMillis:endInMillis:bytesSent:bytesReceived:statusCode:resolver:rejecter:) | ||
| func logNetworkRequest( | ||
| _ url: String, | ||
|
|
@@ -718,4 +716,5 @@ class EmbraceManager: NSObject { | |
|
|
||
| resolve(true) | ||
| } | ||
|
|
||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -544,13 +544,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) | ||
|
|
@@ -566,7 +566,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)) | ||
|
|
@@ -576,22 +576,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) | ||
| } | ||
|
|
||
| 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") | ||
|
|
@@ -601,7 +601,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) | ||
|
|
@@ -649,3 +649,70 @@ class EmbraceSpansSDKNotStartedTests: XCTestCase { | |
| } | ||
| } | ||
|
|
||
| class ComputeBundleIDTests: XCTestCase { | ||
| // https://nshipster.com/temporary-files/ | ||
|
There was a problem hiding this comment. Probably many of this tests under the hood, are using override func tearDownWithError() throws {
UserDefaults.standard.removePersistentDomain(forName: "EmbraceReactNative")
} |
||
| 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) | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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"
UserDefaultsto 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 implementCodablemanually.So, i'd suggest something in the middle, like providing a utility to create an "Embrace" UserDefaults, like this:
And then you can replace
storeandfromStorageusing 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