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

Combine separate *.{android,ios}.js files #3018

Closed
gnprice opened this issue Oct 2, 2018 · 1 comment
Closed

Combine separate *.{android,ios}.js files #3018

gnprice opened this issue Oct 2, 2018 · 1 comment

Comments

@gnprice
Copy link
Member

gnprice commented Oct 2, 2018

We have 8 pairs of files named foo.ios.js / foo.android.js. These make use of an RN feature where when building for each platform, only the one file is included.

We should stop using this feature. As I explained here a little while ago, there are a couple of quite concrete costs to it, and the fact that it's a problem fits a classic pattern:

One reason is this deep dark secret of React Native and Flow, which I noticed recently in our .flowconfig:

[ignore]
; We fork some components by platform
.*/*[.]android.js

(That's verbatim from the template file in RN upstream -- it's not our doing.) In other words, this .android.js / .ios.js convention is totally opaque to Flow, and as a result the best we know how to do is to completely leave .android.js files out from type-checking.

Similarly, the spiffy navigation features in VS Code, to jump to definition or find references, find this totally opaque.

So there's a pretty big cost to using this .android.js/.ios.js feature, as opposed to plain old conditionals.

I think this actually illustrates a kind of classic pattern: once any whiff of the logic of a program starts being expressed in ad-hoc fashion in some kind of configuration outside of the Real Programming Language(s) the rest of the program is written in, it generally isn't long before it begins to hurt to be doing that (a) without the nice, full-featured, composable, semantics of a Real Programming Language and (b) without the benefit of all the tooling that you and your language community have built up around that language (or languages). A very common form of this pattern is templating languages, which either are painfully restrictive or rapidly grow to encompass the main language or both; config-file formats tend to go this way as well.

For us, the Real Programming Language we use for the bulk of the app (including all this code) is JavaScript, and I think plain JS conditionals should express the same thing as the .android.js / .ios.js feature quite nicely.

Instead, we should just use plain JS conditionals.


What's involved in doing so? Here are the 8 such pairs we have:

$ wc -l {.,src/**}/*.@(android|ios).js
    7 ./index.android.js
    7 ./index.ios.js
   20 src/api/downloadFile.android.js
    8 src/api/downloadFile.ios.js
    8 src/api/notifications/registerPush.android.js
    9 src/api/notifications/registerPush.ios.js
    6 src/api/notifications/unregisterPush.android.js
    6 src/api/notifications/unregisterPush.ios.js
  370 src/compose/ComposeBox.android.js
  339 src/compose/ComposeBox.ios.js
    8 src/lightbox/shareImage.android.js
   19 src/lightbox/shareImage.ios.js
   23 src/start/oauth.android.js
   20 src/start/oauth.ios.js
    4 src/utils/openLink.android.js
    4 src/utils/openLink.ios.js

So 7 are very short. These, we should just straightforwardly combine. See
dd7cf10 for an example a little longer than any of those 7, or d0b1317 for a more complex example.

The one not-short example is ComposeBox. That'll be more work, and blocked behind some other work which we're actively pursuing; it's tracked separately, as #3017. (This one is an example where conditionals would have been less appealing... though if we didn't expect to get rid of one version soon, it'd make sense to do some refactoring to make conditionals work well there too.)

@borisyankov
Copy link
Contributor

I'll take on the src/api/* ones, as part of the API PR.

jainkuniya added a commit to jainkuniya/zulip-mobile that referenced this issue Oct 3, 2018
No logical change, just code refactor.

Part of zulip#3018
jainkuniya added a commit to jainkuniya/zulip-mobile that referenced this issue Oct 3, 2018
No logical change, just code refactor. Refer zulip#3018 to know the
motivation behind this.

Create mock `DocumentDir` method for `react-native-fetch-blob` to pass
jest test cases
(wkh237/react-native-fetch-blob#383).
borisyankov added a commit to borisyankov/zulip-mobile that referenced this issue Oct 5, 2018
Part of zulip#3018

Merge the platform specific-files of `downloadFile` into one
file that manually checks for the appropriate version.
gnprice pushed a commit to jainkuniya/zulip-mobile that referenced this issue Oct 11, 2018
No logical change, just code refactor.

Part of zulip#3018
gnprice pushed a commit that referenced this issue Oct 11, 2018
No logical change, just code refactor. Refer #3018 to know the
motivation behind this.

Create mock `DocumentDir` method for `react-native-fetch-blob` to pass
jest test cases
(wkh237/react-native-fetch-blob#383).
gnprice added a commit that referenced this issue Apr 4, 2019
One of the few remaining examples of #3018.

Note that it's definitely fine to import unconditionally from
`react-native-safari-view` on Android... because our `utils/openLink`
already does, and in fact does so in order to provide the exact same
functionality as one of these functions. :-)  So this illustrates
again how the .ios.js / .android.js split can hide opportunities to
share logic.
@gnprice gnprice closed this as completed in 0a3fe65 Apr 4, 2019
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

No branches or pull requests

2 participants