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

[UM][iOS] Fix bool variable representation #4862

Merged
merged 4 commits into from Jul 9, 2019

Conversation

lukmccall
Copy link
Contributor

@lukmccall lukmccall commented Jul 8, 2019

Why

Resolve #4656

How

BOOL type in objective-c can be represented by bool or signed char (according to objc.h).
We only check if BOOL variables are represented as a bool.

Test Plan

Test-suits passed on iPhone 5 and 8 simulator.
Also, you can check https://snack.expo.io/@lukaszkosmaty/[sqlite]-ios-fails-to-execute-%22create-table%22 (in particular, it works on iPhone 5).

@ide ide changed the title [sqlite][iOS] Fix bool variable representation [UM][iOS] Fix bool variable representation Jul 8, 2019
Copy link
Member

@ide ide left a comment

Choose a reason for hiding this comment

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

Can you please provide a more robust test plan that shows the code working?

packages/@unimodules/core/ios/UMCore/UMExportedModule.m Outdated Show resolved Hide resolved
@lukmccall lukmccall requested a review from ide July 9, 2019 08:26
Copy link
Member

@tsapeta tsapeta 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 to me 👍

packages/@unimodules/core/ios/UMCore/UMExportedModule.m Outdated Show resolved Hide resolved
@lukmccall lukmccall force-pushed the @lukmccall/expo-sqlite-create-table-failed branch from ce26812 to 512fde3 Compare July 9, 2019 08:45
@lukmccall
Copy link
Contributor Author

On older devices (with 32 bit iOS) BOOL type is represented by signed char so I tested SQL module on iPhone 5 (simulator and physical device)

@lukmccall lukmccall force-pushed the @lukmccall/expo-sqlite-create-table-failed branch from 512fde3 to c0d7815 Compare July 9, 2019 09:05
@tsapeta
Copy link
Member

tsapeta commented Jul 9, 2019

@lukmccall Could you also add changelog entry for that? Maybe not specifically for SQLite as I can imagine there are some more issues that might have been solved by this.

lukmccall and others added 4 commits July 9, 2019 12:25
@lukmccall lukmccall force-pushed the @lukmccall/expo-sqlite-create-table-failed branch from c0d7815 to b1887aa Compare July 9, 2019 10:35
@tsapeta tsapeta merged commit 38197ed into master Jul 9, 2019
@tsapeta tsapeta deleted the @lukmccall/expo-sqlite-create-table-failed branch July 9, 2019 12:19
@giautm
Copy link
Contributor

giautm commented Jul 17, 2019

Hi @tsapeta , Is it released in SDK-33 as a fix? I facing with this issues on production right now.

@ide
Copy link
Member

ide commented Jul 19, 2019

This will go out with SDK 34.

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.

[sqlite] iOS fails to execute "create table" with "could not prepare..."
4 participants