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

Native Module Docs need to be updated for RCT_EXPORT_MODULE #793

Closed
colinramsay opened this issue Apr 10, 2015 · 11 comments
Closed

Native Module Docs need to be updated for RCT_EXPORT_MODULE #793

colinramsay opened this issue Apr 10, 2015 · 11 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@colinramsay
Copy link
Contributor

Previously we'd use RCT_EXPORT but it seems that there's been a change (9th Apr) that means we need to use RCT_EXPORT_MODULE on the class and RCT_EXPORT_METHOD for each method to be exported. The docs don't yet reflect this. It also raises two points:

  • What's the policy for informing the community of breaking changes synced from FB?
  • Should changes be synced without associated documentation?
@jaygarcia
Copy link
Contributor

^-- Those are fantastic questions.

I've been reading the PRs , especially ones coming from the FB team that say "Updates ...."

Another point, i'd like to add to Colin's:

  • Can the repo owners make it a priority to update docs before pushing out such changes?

@tptee
Copy link

tptee commented Apr 10, 2015

Do these changes resolve #579 in any way?

@ide
Copy link
Contributor

ide commented Apr 10, 2015

Also, if FB has codemod regexes for the upgrade it'd be helpful to get those.

@nicklockwood
Copy link
Contributor

We codemodded manually ¯_(ツ)_/¯

The old RCT_IMPORT() syntax for methods should still be supported for a while though, so no need to upgrade everything right away.

@lwansbrough
Copy link
Contributor

👍 This is super annoying. I know we're still in very early development but I'm having to fix breaking changes for every single patch version that's released. Would be great if the docs were updated whenever breaking changes are introduced.

@brentvatne
Copy link
Collaborator

@colinramsay - fixed in #801

@colinramsay
Copy link
Contributor Author

Splendid, thanks!

On Fri, Apr 10, 2015 at 10:38 PM, Brent Vatne notifications@github.com
wrote:

@colinramsay - fixed in #801

Reply to this email directly or view it on GitHub:
#793 (comment)

@nicklockwood
Copy link
Contributor

@lwansbrough I understand that this is frustrating, and we'll try to do better, particularly with the docs updates.

Release notes are problematic due to the way that fixes are landed internally and then pushed to github in batches. Again, we're actively working to improve that process.

I would like to reassure you that there is some method to the madness though. We're aware that breaking changes are painful, and we'll do our best to a) avoid making them, and b) when we do make them, ensure that we break things in an obvious way, rather than having things silently fail.

For example, in the change to module registration, we left the class scanning feature enabled in debug mode, purely so we could detect modules that have not yet been converted, and provide a useful error message.

Similarly, for the RCT_EXPORT -> RCT_EXPORT_METHOD change, we've left in full support for the old system so that you can make the upgrades at your convenience instead of immediately breaking the apps.

I'm pretty happy with the technical approach we've taken here, but we'll try to improve on the communication and documentation side too.

@colinramsay
Copy link
Contributor Author

The old approach doesn't seem to work, I tried it myself yesterday with the same result. 

http://stackoverflow.com/q/29552377/125680

On Sat, Apr 11, 2015 at 9:08 AM, Nick Lockwood notifications@github.com
wrote:

@lwansbrough I understand that this is frustrating, and we'll try to do better, particularly with the docs updates.
Release notes are problematic due to the way that fixes are landed internally and then pushed to github in batches. Again, we're actively working to improve that process.
I would like to reassure you that there is some method to the madness though. We're aware that breaking changes are painful, and we'll do our best to a) avoid making them, and b) when we do make them, ensure that we break things in an obvious way, rather than having things silently fail.
For example, in the change to module registration, we left the class scanning feature enabled in debug mode, purely so we could detect modules that have not yet been converted, and provide a useful error message.
Similarly, for the RCT_EXPORT -> RCT_EXPORT_METHOD change, we've left in full support for the old system so that you can make the upgrades at your convenience instead of immediately breaking the apps.

I'm pretty happy with the technical approach we've taken here, but we'll try to improve on the communication and documentation side too.

Reply to this email directly or view it on GitHub:
#793 (comment)

@nicklockwood
Copy link
Contributor

Sorry if I was unclear: RTC_EXPORT_MODULE() is required, but RTC_EXPORT_METHOD() shouldn't be. These landed around the same time, but they're distinct changes.

(i.e if the old RCT_EXPORT() approach of exporting methods isn't working, that's a bug, not deliberate).

@brentvatne
Copy link
Collaborator

@facebook facebook locked as resolved and limited conversation to collaborators May 29, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

8 participants