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.

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(iOS): properly handle date types during JSValue coercion #4043

Merged
merged 13 commits into from
Jan 13, 2021

Conversation

theproducer
Copy link
Contributor

@theproducer theproducer commented Jan 11, 2021

This PR fixes a missing case in coerceToJSValue in which NSDate values were not being returned, resulting in nil values.

Example:

const mydate = new Date();
const result = await ExamplePlugin.testMethod({ date: mydate });
@objc func testMethod(_ call: CAPPluginCall) {
         // date will be nil
        let date = call.getDate("date")
        
       
        if let date = date {
            print("Date: \(date)")
        }
    }

@theproducer theproducer marked this pull request as ready for review January 11, 2021 19:29
@theproducer theproducer requested a review from ikeith January 11, 2021 19:30
Copy link
Contributor

@ikeith ikeith left a comment

Choose a reason for hiding this comment

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

Looks good; just some minor changes. And please remove the unrelated whitespace diffs, if possible.

ios/Capacitor/Capacitor/JSTypes.swift Outdated Show resolved Hide resolved
ios/Capacitor/Capacitor/JSTypes.swift Outdated Show resolved Hide resolved
ios/Capacitor/Capacitor/JSTypes.swift Outdated Show resolved Hide resolved
@theproducer theproducer requested a review from ikeith January 11, 2021 23:35
ios/Capacitor/Capacitor/JSTypes.swift Outdated Show resolved Hide resolved
@imhoffd imhoffd added this to the 3.0.0-beta milestone Jan 12, 2021
@theproducer theproducer requested a review from ikeith January 12, 2021 23:18
Copy link
Contributor

@ikeith ikeith left a comment

Choose a reason for hiding this comment

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

Looks good. The type bridging actually has a lot of test coverage (the exception in the project) so it'd be good to update the unit tests to verify the additional date handling. But that can be addressed in a separate branch.

@imhoffd imhoffd merged commit 1affae7 into main Jan 13, 2021
@imhoffd imhoffd deleted the coerce-date-types branch January 13, 2021 00:07
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