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

[core] Removed all NativeModulesProxy occurrences #31496

Merged
merged 9 commits into from
Sep 17, 2024

Conversation

reichhartd
Copy link
Contributor

@reichhartd reichhartd commented Sep 16, 2024

Why

In order to remove NativeModulesProxy from expo-modules-core in the future, I have replaced the use of it in all other packages.

How

I have replaced all occurrences of the deprecated NativeModulesProxy.

I have left the export of NativeModulesProxy in the index.ts of expo-modules-core, but can remove it if it is not too early.

Test Plan

Test cases run.

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Sep 16, 2024
@reichhartd reichhartd changed the title [core] Remove all NativeModulesProxy occurrences [core] Removed all NativeModulesProxy occurrences Sep 16, 2024
@reichhartd reichhartd force-pushed the fix/remove-native-proxy branch from 561e4bb to 6e2306c Compare September 16, 2024 13:24
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Sep 16, 2024
# Conflicts:
#	packages/expo-modules-core/CHANGELOG.md
@reichhartd reichhartd marked this pull request as ready for review September 16, 2024 13:33
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.

Great, thank you so much! 🙏 I left some changelog suggestions, but in general it looks good.
Recently I had some troubles to move off of NativeModulesProxy in expo-font tests, but looks like they are passing now – which is great 👍

packages/expo-modules-core/CHANGELOG.md Outdated Show resolved Hide resolved
packages/expo-screen-orientation/CHANGELOG.md Outdated Show resolved Hide resolved
packages/expo-secure-store/CHANGELOG.md Outdated Show resolved Hide resolved
packages/expo-sms/CHANGELOG.md Outdated Show resolved Hide resolved
packages/expo-constants/CHANGELOG.md Outdated Show resolved Hide resolved
packages/expo-font/CHANGELOG.md Outdated Show resolved Hide resolved
packages/expo-maps/CHANGELOG.md Outdated Show resolved Hide resolved
reichhartd and others added 7 commits September 17, 2024 00:14
Co-authored-by: Tomasz Sapeta <tomasz.sapeta@me.com>
Co-authored-by: Tomasz Sapeta <tomasz.sapeta@me.com>
Co-authored-by: Tomasz Sapeta <tomasz.sapeta@me.com>
Co-authored-by: Tomasz Sapeta <tomasz.sapeta@me.com>
Co-authored-by: Tomasz Sapeta <tomasz.sapeta@me.com>
Co-authored-by: Tomasz Sapeta <tomasz.sapeta@me.com>
Co-authored-by: Tomasz Sapeta <tomasz.sapeta@me.com>
@wschurman wschurman removed their request for review September 16, 2024 16:02
@reichhartd
Copy link
Contributor Author

You're welcome 🙏
Is it too early to remove the export of NativeModulesProxy in the index.ts of expo-modules-core? Since it would be a breaking change.

@tsapeta
Copy link
Member

tsapeta commented Sep 17, 2024

Yeah, I think it's too early. We'll probably need to search over GitHub to see if there are any popular libraries importing it and help them migrate. I would keep it until we entirely remove the legacy modules from the native code.

One more thing we could do is to make it a Proxy object and log a warning in its getter, so it's easier to identify places where the deprecated API is used.

@tsapeta tsapeta merged commit 17c3747 into expo:main Sep 17, 2024
8 of 10 checks passed
@reichhartd reichhartd deleted the fix/remove-native-proxy branch September 18, 2024 12:40
@brentvatne brentvatne added the published Changes from the PR have been published to npm label Oct 22, 2024
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 published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants