Skip to content

[Bridge] RCT_REMAP_METHOD(js_name, selector) #802

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

Closed
wants to merge 1 commit into from

Conversation

ide
Copy link
Contributor

@ide ide commented Apr 10, 2015

cc @a2 @nicklockwood

This diff introduces a new macro called RCT_EXPORT_NAMED_METHOD, which is like RCT_EXPORT_METHOD but lets you choose the name of the method in JS. This diff is backwards compatible with the RCT_EXPORT_METHOD and legacy RCT_EXPORT macros.

The entries in the data segment now contain __func__, the Obj-C selector signature, and the JS name. If the JS name is NULL, we take the legacy RCT_EXPORT code path. If the JS name is an empty string, we use the Obj-C selector's name up to the first colon (that is, the behavior of RCT_EXPORT_METHOD).

Since there are three values in each data segment entry, the macros now specify 1-byte alignment. Without the byte alignment, the compiler defaults to 2-byte alignment meaning that each entry takes up 4 bytes instead of 3. The extra byte isn't a concern but being explicit about the alignment should reduce compiler surprises.

@ide ide force-pushed the named-exported-methods branch from 650a24f to 1eabe61 Compare April 10, 2015 21:12
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 10, 2015
@ide ide force-pushed the named-exported-methods branch from 1eabe61 to 40c1d68 Compare April 10, 2015 21:16
@ide
Copy link
Contributor Author

ide commented Apr 10, 2015

If this looks good I'll send another PR to update the docs once this lands.

@nicklockwood
Copy link
Contributor

We did debate adding this at the time of the refactor. In the end I decided it wasn't really necessary - these methods are written explicitly for the purpose of exporting to JavaScript, so there's no need to maintain compatibility with existing code, and the approach of taking just the first part of the selector allows full freedom to name it whatever you want.

The only disadvantage is that it forces you to sometimes name your method slightly un-idiomatically with respect to Objective-C - i.e. you might have to call it:

- (void)doSomething:(id)foo bar:(id)bar;

Instead of:

- (void)doSomethingWithFoo:(id)foo bar:(id)bar;

Which would be the convention. This does bother me a little bit, but honestly so few people will care about this that it hardly seems worth increasing the API surface to support it. We checked our existing modules and there were only about 2 cases in the whole project where someone had bothered to use a different name, even though this was fully supported under the old system.

@nicklockwood
Copy link
Contributor

Another solution I considered was to do the Swift-like thing of removing the WithX: from the first selector, if present. Then you could write

- (void)doSomethingWithFoo:(id)foo bar:(id)bar;

And the JS method would still be. doSomething().

The reason I decided against that is that, while it would make Objective-C people happier, it would cause more confusion for JS developers, and we want to avoid that since the majority of our target audience at the moment are not expert native developers.

@nicklockwood
Copy link
Contributor

Another thought:

For properties, where we do support something similar, we call the macro RCT_REMAP_VIEW_PROPERTY(). I don't love this, but it if we were to add such a feature, I feel we should try to be consistent with the naming.

@ide
Copy link
Contributor Author

ide commented Apr 11, 2015

Renaming it to RCT_REMAP_METHOD is alright with me. I had a need for this in a database module where the Obj-C selectors looked like:

- (void)executeQuery:(NSString *)query parameterDictionary:(NSDictionary *)parameters successCallback:...;
- (void)executeQuery:(NSString *)query argumentsArray:(NSArray *)array successCallback:...;

These are exported to JS as executeQueryWithParameters and executeQueryWithArgumentArray respectively. I was much more satisified with named exports than awkwardly writing

- (void)executeQueryWithParameters:(NSString *)query parameterDictionary:(NSDictionary *)parameters successCallback:...

@ide ide force-pushed the named-exported-methods branch from 40c1d68 to 45e877f Compare April 11, 2015 08:36
@ide ide changed the title [Bridge] RCT_EXPORT_NAMED_METHOD(js_name, selector) [Bridge] RCT_REMAP_METHOD(js_name, selector) Apr 11, 2015
@nicklockwood
Copy link
Contributor

Eh.. fair enough :-)

This diff introduces a new macro called `RCT_REMAP_METHOD`, which is like `RCT_EXPORT_METHOD` but lets you choose the name of the method in JS. This diff is backwards compatible with the `RCT_EXPORT_METHOD` and legacy `RCT_EXPORT` macros.

The entries in the data segment now contain `__func__`, the Obj-C selector signature, and the JS name. If the JS name is `NULL`, we take the legacy `RCT_EXPORT` code path. If the JS name is an empty string, we use the Obj-C selector's name up to the first colon (that is, the behavior of `RCT_EXPORT_METHOD`).

Since there are three values in each data segment entry, the macros now specify 1-byte alignment. Without the byte alignment, the compiler defaults to 2-byte alignment meaning that each entry takes up 4 bytes instead of 3. The extra byte isn't a concern but being explicit about the alignment should reduce compiler surprises.
@ide ide force-pushed the named-exported-methods branch from c21eef6 to dcb54ec Compare April 14, 2015 00:19
vjeux pushed a commit to vjeux/react-native that referenced this pull request Apr 14, 2015
Summary:
cc @a2 @nicklockwood

This diff introduces a new macro called `RCT_EXPORT_NAMED_METHOD`, which is like `RCT_EXPORT_METHOD` but lets you choose the name of the method in JS. This diff is backwards compatible with the `RCT_EXPORT_METHOD` and legacy `RCT_EXPORT` macros.

The entries in the data segment now contain `__func__`, the Obj-C selector signature, and the JS name. If the JS name is `NULL`, we take the legacy `RCT_EXPORT` code path. If the JS name is an empty string, we use the Obj-C selector's name up to the first colon (that is, the behavior of `RCT_EXPORT_METHOD`).

Since there are three values in each data segment entry, the macros now specify 1-byte alignment. Without the byte alignment, the compiler defaults to 2-byte alignment meaning that each entry takes up 4 bytes instead of 3. The extra byte isn't a concern but being explicit about the alignment should reduce compiler surprises.
Closes facebook#802
Github Author: James Ide <[email protected]>

Test Plan: Imported from GitHub, without a `Test Plan:` line.
@ide ide closed this in e193a13 Apr 15, 2015
@ide ide deleted the named-exported-methods branch April 15, 2015 21:00
@ide
Copy link
Contributor Author

ide commented Apr 15, 2015

Merged in 862d9fb.

ayushjainrksh referenced this pull request in MLH-Fellowship/react-native Jul 2, 2020
* add fontFamily note for Text

* Typos
facebook-github-bot pushed a commit that referenced this pull request Apr 11, 2022
…it (#802)

Summary:
## Summary
The PR is essentially to update [async](https://www.npmjs.com/package/async) to version [3.2.2](https://github.com/caolan/async/blob/master/CHANGELOG.md#v322) to fix t a  [prototype pollution exploit](https://security.snyk.io/vuln/SNYK-JS-ASYNC-2441827) found in versions < `3.2.2` . The vulnerability was discovered by [Snyk](https://snyk.io/) has discovered an exploit in  and labelled as **High Severity**.

Changelog: [Internal]

X-link: facebook/metro#802

Reviewed By: GijsWeterings

Differential Revision: D35543054

Pulled By: robhogan

fbshipit-source-id: b176c584dbcb139115e466a765e3efbe6f1f984d
mganandraj pushed a commit to mganandraj/react-native that referenced this pull request May 13, 2022
…it (facebook#802)

Summary:
The PR is essentially to update [async](https://www.npmjs.com/package/async) to version [3.2.2](https://github.com/caolan/async/blob/master/CHANGELOG.md#v322) to fix t a  [prototype pollution exploit](https://security.snyk.io/vuln/SNYK-JS-ASYNC-2441827) found in versions < `3.2.2` . The vulnerability was discovered by [Snyk](https://snyk.io/) has discovered an exploit in  and labelled as **High Severity**.

Changelog: [Internal]

X-link: facebook/metro#802

Reviewed By: GijsWeterings

Differential Revision: D35543054

Pulled By: robhogan

fbshipit-source-id: b176c584dbcb139115e466a765e3efbe6f1f984d
mganandraj pushed a commit to mganandraj/react-native that referenced this pull request May 13, 2022
…it (facebook#802)

Summary:
The PR is essentially to update [async](https://www.npmjs.com/package/async) to version [3.2.2](https://github.com/caolan/async/blob/master/CHANGELOG.md#v322) to fix t a  [prototype pollution exploit](https://security.snyk.io/vuln/SNYK-JS-ASYNC-2441827) found in versions < `3.2.2` . The vulnerability was discovered by [Snyk](https://snyk.io/) has discovered an exploit in  and labelled as **High Severity**.

Changelog: [Internal]

X-link: facebook/metro#802

Reviewed By: GijsWeterings

Differential Revision: D35543054

Pulled By: robhogan

fbshipit-source-id: b176c584dbcb139115e466a765e3efbe6f1f984d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants