Skip to content
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.

Sign up for GitHub

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

Queue Events for better media uploads #2366

Merged
merged 11 commits into from
Jun 18, 2020
Merged

Queue Events for better media uploads #2366

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
83541c1
Setup handshake mechanism for gutenberg bridge
chipsnyder Jun 9, 2020
f487990
Simplify handshake
chipsnyder Jun 9, 2020
e01ea90
Adjust call for replaying events to call super to avoid the custom lo…
chipsnyder Jun 10, 2020
880478c
Revert unneeded changes and remove mocked event
chipsnyder Jun 10, 2020
04e3038
Adjust threading and method for queue
chipsnyder Jun 12, 2020
85c1770
Revert gutenberg change
chipsnyder Jun 12, 2020
fb8dad5
Bump gutenberg version
chipsnyder Jun 15, 2020
d59599a
Merge remote-tracking branch 'origin/develop' into issue/2325-bridge
chipsnyder Jun 15, 2020
7f1bc5a
Adjust notification name to use existing extensions
chipsnyder Jun 16, 2020
7997acb
Merge remote-tracking branch 'origin/develop' into issue/2325-bridge
chipsnyder Jun 17, 2020
8f13252
Adjust optionality
chipsnyder Jun 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gutenberg
Open in desktop
Submodule gutenberg updated 237 files
4 changes: 2 additions & 2 deletions react-native-gutenberg-bridge/ios/Gutenberg.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public class Gutenberg: NSObject {
if let serverID = serverID {
data["mediaServerId"] = serverID
}
bridgeModule.sendEventIfNeeded(.mediaUpload, body: data)
sendEvent(.mediaUpload, body: data)
}

public func appendMedia(id: Int32, url: URL, type: MediaType) {
Expand All @@ -135,7 +135,7 @@ public class Gutenberg: NSObject {
"mediaUrl" : url.absoluteString,
"mediaType": type.rawValue,
]
bridgeModule.sendEventIfNeeded(.mediaAppend, body: data)
sendEvent(.mediaAppend, body: data)
}

public func setFocusOnTitle() {
Expand Down
44 changes: 35 additions & 9 deletions react-native-gutenberg-bridge/ios/RNReactNativeGutenbergBridge.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,24 @@
struct GutenbergEvent {
let name: String
let body: Any
}

etoledom marked this conversation as resolved.
Show resolved Hide resolved
@objc (RNReactNativeGutenbergBridge)
public class RNReactNativeGutenbergBridge: RCTEventEmitter {
weak var delegate: GutenbergBridgeDelegate?
weak var dataSource: GutenbergBridgeDataSource?
private var isJSLoading = true
private var hasObservers = false
private var queuedEvents = [GutenbergEvent]()

public override init() {
super.init()
NotificationCenter.default.addObserver(forName: .RCTContentDidAppear, object: nil, queue: nil) { (_) in
DispatchQueue.main.async {
self.connectionEstablished()
}
}
}
// MARK: - Messaging methods

@objc
Expand Down Expand Up @@ -87,7 +101,9 @@ public class RNReactNativeGutenbergBridge: RCTEventEmitter {
@objc
func mediaUploadSync() {
DispatchQueue.main.async {
self.delegate?.gutenbergDidRequestMediaUploadSync()
if self.hasObservers {
self.delegate?.gutenbergDidRequestMediaUploadSync()
}
}
}

Expand Down Expand Up @@ -188,18 +204,28 @@ public class RNReactNativeGutenbergBridge: RCTEventEmitter {
}
}

private func shouldLog(with level: Int) -> Bool {
return level >= RCTGetLogThreshold().rawValue
public func connectionEstablished() {
guard !hasObservers else { return } // We have an established connection no need to continue.
hasObservers = true
etoledom marked this conversation as resolved.
Show resolved Hide resolved
while (self.queuedEvents.count > 0) {
let event = self.queuedEvents.removeFirst()
super.sendEvent(withName: event.name, body: event.body) // execute this on super as we want to avoid logic in self.
}
}

override public func startObserving() {
super.startObserving()
hasObservers = true
public override func sendEvent(withName name: String!, body: Any!) {
DispatchQueue.main.async {
if self.hasObservers && self.queuedEvents.count == 0 {
super.sendEvent(withName: name, body: body)
} else {
let event = GutenbergEvent(name: name, body: body)
Copy link
Contributor

@etoledom etoledom Jun 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry! I meant the warning on this line. I wrote the comment but it disappeared somehow (second time that happens to me this week ¯\_(ツ)_/¯ )

The warning says Coercion of implicitly unwrappable value of type 'Any?' to 'Any' does not unwrap optional

Would the override still work if we change the optionality of the parameters?
Actually on a second look, that Any! looks quite dangerous. This might crash if any queued event had no body.

It would be great if name were not optional and body is an actual non-implicitly-unwrapped optional, but open to possibilities too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just did a test and it doesn't crash. Any seems to accept a nil value even if it's not an optional itself (?)

But still... I wouldn't trust the runtime completely with this 😬

Copy link
Contributor

@mkevins mkevins Jun 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I just did a test and it doesn't crash. Any seems to accept a nil value even if it's not an optional itself (?)

Interesting! I wouldn't have expected that. Does that mean the struct's default initializer is not actually force-unwrapping the value? When you tested with a nil, do you know if the GutenbergEvent was initialized, or did the code branch directly to the super.sendEvent?

Update:

In a playground, I just tried this:

struct SomeData {
    let thing1: String
    let thing2: Any
}

func test(t1: String, t2: Any!) {
    var nilThing: Any

    let data = SomeData(thing1: t1, thing2: t2)
    print(data.thing1)

    // no errors
    nilThing = t2
    nilThing = data.thing2
}

test(t1: "Hello world!", t2: nil)

And there are no errors in runtime. Interestingly, when I use AnyObject, I get a run-time crash (Terminated by signal 4), just initializing the struct:

struct SomeData {
    let thing1: String
    let thing2: AnyObject
}

func test(t1: String, t2: AnyObject!) {
    let data = SomeData(thing1: t1, thing2: t2)
}

test(t1: "Hello world!", t2: nil)

Perhaps there is some subtlety with permissiveness afforded for potential value or reference types? 🤔

Copy link
Contributor

@etoledom etoledom Jun 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps there is some subtlety with permissiveness afforded for potential value or reference types? 🤔

This kinds of make sense in a way that Optional is basically a Value type, and we are passing Optional<>.none as Any.

And AnyObject wouldn't accept it since it expects Reference types only, so it will try to unwrap the optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation Eduardo! I also found this explanation, which is just as you mentioned. The discussion is interesting because it explains why you cannot directly assign nil to a var with type Any, since there is no inferred type for the generic in the Optional. That there is no error in the above case was surprising to me, but this explains it (kind of a language design edge case, I suppose).

Copy link
Contributor

@etoledom etoledom Jun 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

So @mkevins @chipsnyder , what do you think: Should we avoid this edge case?

I think it would be good to try avoid it, but since it's clear now that it won't crash, maybe it's not a blocker for this PR. Still it would be good to clear the warning 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment but it disappeared somehow

Oh, that is weird! Thanks for confirming though.

The warning says Coercion of implicitly unwrappable value of type 'Any?' to 'Any' does not unwrap optional

I totally missed this error thank you for catching it. Also thank you both for the investigation and knowledge share. That's really interesting on how Any and Optionals interact.

(kind of a language design edge case, I suppose).

Yeah this is neat, sometimes it's easy to forget that language design has its own edge cases to deal with.

what do you think: Should we avoid this edge case?

To me, it seems like an easy fix to adjust. So I'll make that tweak today and push up a commit to this PR here shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actually an interesting situation. @etoledom I didn't run into the warning that you were seeing so I was wondering if it was a runtime flag, but I wasn't able to see it at runtime either.

I do think it would be a good idea to be explicit about the optionality of the event parameters though so I did an update to address that. If you don't mind will you let me know if it fixes the warning?

XCode 11.5
Screen Shot 2020-06-17 at 12 38 24 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Fwiw, I didn't see the warning either (but I was exploring the code via the WordPress-iOS project, so I wonder if that's a factor?)

Copy link
Contributor

@etoledom etoledom Jun 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Oh right, you will see it if you run the gutenberg example project on Xcode.

EDIT:
The last change from 8f13252 solved the warning!

Thank you @chipsnyder for the update 🙏

self.queuedEvents.append(event)
}
}
}

override public func stopObserving() {
super.stopObserving()
hasObservers = false
private func shouldLog(with level: Int) -> Bool {
return level >= RCTGetLogThreshold().rawValue
}

@objc
Expand Down