Skip to content

Conversation

tsheaff
Copy link
Contributor

@tsheaff tsheaff commented Jul 2, 2022

Why

Please see #18077 for full motivation

How

Update the types of executeSql > args param from (number | string)[] to (number | string | null)[]

Test Plan

There is already a test passing for this behavior in the test suite! See here. This test is in JS not TS, so no issue was raised on the type mismatch.

Checklist

  • Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md).
  • This diff will work correctly for expo build (eg: updated @expo/xdl).
  • This diff will work correctly for expo prebuild & EAS Build (eg: updated a module plugin).

@tsheaff tsheaff requested a review from bbarthec as a code owner July 2, 2022 00:45
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Jul 2, 2022
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Jul 2, 2022
@tsheaff
Copy link
Contributor Author

tsheaff commented Jul 3, 2022

The test suite errors look to my eyes like red herrings. But caveat: I haven't contributed to this repo before.

But on all three platforms the failure is the same: the test_suite_container doesn't exist when it should. E.g. on Android it fails on await detoxExpect(element(by.id('test_suite_container'))).toExist(); and on web on Element div[data-testid="test_suite_container"] not found. Nothing in this diff seems likely to have caused this issue. I don't have permissions to rerun the tests.

Copy link
Contributor

@bbarthec bbarthec left a comment

Choose a reason for hiding this comment

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

Hello @tsheaff 👋
Thank you for this contribution 😁
It looks good, so I'll merge it right away 🚀
Don't worry about the CI being red - we're having some problems with it lately 😅

@tsheaff
Copy link
Contributor Author

tsheaff commented Jul 6, 2022

Thanks @bbarthec !

How often do you reelase new version of expo-sqlite on npm? e.g. how long until this change is available via NPM?

@bbarthec
Copy link
Contributor

bbarthec commented Jul 6, 2022

@tsheaff, I believe expo-sqlite will be released in the next two weeks 👀

Kudo pushed a commit that referenced this pull request Nov 10, 2023
# Why

#18078 fixed this for the callback-based functions, but not for `executeSqlAsync`. Changing the parameters for the `args` of that function to match.
onizam95 pushed a commit to onizam95/expo-av-drm that referenced this pull request Jan 15, 2024
# Why

expo#18078 fixed this for the callback-based functions, but not for `executeSqlAsync`. Changing the parameters for the `args` of that function to match.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants