search

facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
118.61k stars 24.29k forks source link

feat(CLI) - React Native Cascading Platforms #11408

Closed GantMan closed 7 years ago

GantMan commented 7 years ago

What are you proposing Gant?

Simply put: somefile.ios10.js would be able to take advantage of iOS10 feature where applicable, but then use ios.js files in all other places.

Alas! Platforms can be identified by each project, and don't need to be part enumerated by the react-native core. What's next for platforms? Exposing the native cascade is what I would like to add.

Currently Packager works like so: require('blah') does some neato cascading where blah.somePlatform.js will be chosen over blah.native.js, and ultimately chosen over blah.js. This feature is not exposed for config in a per-project basis. That's what I'd like to change.

Utility

In some situations there's been talk about how platform-transparency will help unify code. Allowing access to the platform specific magic, is a natural second step to allowing access to custom platforms.

This means we wouldn't need to duplicate ALL *.ios.js files as *.ios10.js files just to take advantage in one spot. Perhaps even useful in AppleTV platform, keeping the high duplication JS files to a minimum. 100% this is needed in Windows which is hoping to support WPF and UWP for React Native.

Proposed Method

Much like platforms can be sent as a config via the CLI or by settings in the rn-cli.config.js, we pass our node-haste cascade.

...
getPlatforms() {
  return ['ios10'];
},
hasteCascade() {
  return ['ios'];
}
...

run project for basic ios Everything behaves as normal.
run project for ios10 It will consume any 'ios10' files (platform first!), but cascade into looking for normal .ios.js files before settling for native, and then nondenominational js files, and then result in error for missing files.

TADAA! no need to duplicate all .ios files!

I'm ready to knock this code out for RN. But before I start, I need a few RN heroes to sign off.

cpojer commented 7 years ago

cc @lelandrichardson

davidaurelio commented 7 years ago

I am not quite sure what your mental model for these extension-based switches is – are you assuming that we are switching at runtime or at build time?

Since the switch happens at build time, you would need separate bundles for ios < 10 and ios ≥ 10.. Can you elaborate which scenario you are targeting?

davidaurelio commented 7 years ago

I kind of understand the need for platform cascades in the windows context, e.g. @rozele’s proposal for .xbox.js > .windows.js > .native.js > .js.

If it’s possible to solve this scenario without making the code more complicated, it might be worth considering it.

Could everybody interested post some use cases, please?

Product engineers usually love the platform extensions, the packager team not so much ;-)

Talking about the configuration: We really should find a way not to add the hasteCascade() method (and how would that work with multi-platform projects?).

Something like getPlatforms() { return [ ['xbox', 'windows'], ['winphone', 'windows'], 'windows' ]; } seems easier to configure to me. Even if it’s not great.

rozele commented 7 years ago

@davidaurelio that is precisely my use case Xbox > Windows > Native, I agree that we adding the extra hasteCascade method may not be necessary. Your suggestion for a richer return object from getPlatforms seems reasonable.

douglowder commented 7 years ago

@GantMan just to make the dependencies clearer to developers, how about this naming scheme?

cpojer commented 7 years ago

I agree with @davidaurelio, "getPlatformPriorities" or something that lists fallbacks like this:

getPlatformPriorities() {
  return {
    xbox: ['windows'],
  };
}

might be worth it. I'm a bit concerned about the performance of resolving modules though. I want to make sure we don't make this more complex than it needs to be. If something is the same platform, it may make sense to ship a full bundle and do runtime detection instead; that seems way simpler.

GantMan commented 7 years ago

@davidaurelio - worth noting I got quite a positive reaction from suggesting "*.ipad.js". Which is a common concern/need.

👍 💯 on the above info: to @davidaurelio's note, agreed it's key that platforms don't step on each other, that's a perfect insight.

Considering platform configuration, does it make sense to continue to put everything in rn-cli.config.js Or would it be better to have a "platform file" to avoid platform conflicts? That might be too far too soon, but just a consideration.

cpojer commented 7 years ago

One thing we have to consider is the potential downstream effects on other tools, like Jest or Flow, which do not know anything about resolving these kind of modules so we'd have to also update them. This alone might be enough reason that a runtime switch might be preferred if we expect the platform differences not to be significant that the bundle size matters.

lelandrichardson commented 7 years ago

I haven't had a ton of time to really think about this, but let me try to write down a few points that I think are important for us to think about.

One thing we have to consider is the potential downstream effects on other tools, like Jest or Flow

This is critical. We need to make sure to keep our eye on the bigger picture as well. React Native is becoming a bigger and bigger chunk of the overall JS community every day, and decisions made here can have both a positive and a negative impact on the community overall.

"runtime" vs "compile" time resolution

So far the platform extensions (ios/android/web/vr/etc.) have all been done in such a way that each extension would result in an entirely new bundle, which was fine, because each platform would be published separately. By having ios.10.js, we now have a full matrix of bundles that we will need to create and potentially deploy together. Without a solid bundle splitting solution, I worry about this creating bloat on the device.

Similarly, if we are using version numbers (ie, ios10 vs ios9), this get's even stranger. What happens when the use upgrades to ios11 without updating their app? What if what you really mean is "iOS versions greater than or equal to 10"? For Android, I could see this quickly getting out of control.

I think for these use cases, it may be better to just use the Platform API. For instance:

// Foo.js
import { Platform } from 'react-native';

module.exports = Platform.select({
  android: () => require('./Foo-android'),
  ios: () => Platform.Version >= 10 
    ? require('./Foo-advanced-ios'),
    : require('./Foo-simple-ios'),
})();

This achieves the same goals, but is resolved at runtime. With this configuration, you'd end up packaging all three versions, but only one would get executed. This is good so long as the union dependency trees of these files is not that different (which, since they are the same platforms, just different versions, is mostly going to be the case).

However, I believe for entirely different platforms, the need to do the compile time resolution is more important (for instance on "web" vs. "native", I could see the dependency trees diverging quite a bit, and we wouldn't (for example) want to bundle a big dependency like jquery into a mobile app, even if it never got executed.

Composable configuration...

One thing that I want to make sure we accomplish with whatever solution we come up with is a sane way to make these configurations composable. We need to have a path for library developers to be able to provide the same platform extension toggles without conflicting with the platform toggles that the developer in the consuming app specifies.

Make available to non-RN tooling

And on a related note, If I create a <FancyButton /> component for RN, and i decide that there's a way for me to create the same thing for web, I should be able to specify a .web.js version of it, and have tools like webpack/browserify/rollup/etc. be able to figure out how to use it instead of the RN version. This may only be possible with a webpack plugin or something, but we should be thinking about this. Additionally, we should pay close attention to the discussion in #10966 for related points to this.

cc @ljharb for thoughts on this

GantMan commented 7 years ago

awesome feedback @lelandrichardson ! I couldn't agree more, this is the time to really evaluate the best practices. Platforms need this support, and it should be done well.

Quick question

The bundle problem - Agreed. A matrix of bundles is a bad solution. Right now, you get .js files, and platform js files no problem. So bundling for xbox would simply bundle along the cascade, kind of like we already do. If it works in packager, I feel it's transparent in the bundle. Am I wrong?In the current system if I have *.ios.js and *.js do they both end up in the bundle?

Quick note

The developer experience is key. If I do <Text>, it shouldn't be the developer's job know I need to import differently for something so fundamental, just because they are targeting a different platform.

The simple method we're using for existing platforms should behave the same as new platforms integrate.

matthargett commented 7 years ago

totally agree with both @lelandrichardson and @rozele , but just to explicitly state the goals/preferences for our (BlueJeans) portfolio of React Native projects across multiple platforms:

  1. 100% agree with making sure Flow (and IDEs like WebStorm and ReSharper) can continue to correctly navigate across these configurable switches for inter-module data flow analysis. Real static analysis (not just simple style checkers) is a critical part of our overall CI, CD, and DX strategy.

  2. We would prefer compile-time resolution so we can do separate bundles that keep our initial app download as small as possible. We have very aggressive size targets on client download so the initial times to get into a BlueJeans meeting are very, very short.

  3. Form factor/DPI/widget specialization is more important to us than strict OS versioning, though there is often overlap. I'm very open to other ideas/suggestions that meet the ultimate needs of paring down download size while improving code sharing. This level of resolution is less important for our future RN Linux work, as that is anticipated to be standard desktop/laptop.

  4. We are planning on sharing lots of non-UI code between our RN client and Web frontend, and would love a unified way of sharing UI components across web and RN as well. That being said, items 0-2 are more critical to us for scaling the team and long-term sustainability of our native client codebases.

mkonicek commented 7 years ago

I'll let @cpojer and @davidaurelio make the call here.

somefile.ios10.js

I think it might be easier to check at runtime (using Platform?) what version of the OS you're running on. Probably no need to fork and package different files based just on OS version? No strong opinion here but would be nice to keep this simple and not have an explosion of lots of extensions with complex cascading rules in RN projects. That might make those codebases hard to understand.

Agree that we need a nice generic solution for .windows.js files (and maybe other platforms?) instead of the few hardcoded places in the packager to support .windows.js files.

GantMan commented 7 years ago

Update: This centralized config update is awesome, and fits in vein for project config https://github.com/facebook/react-native/pull/11564

Circling back in a few days after some analysis for another ticket update.

GantMan commented 7 years ago

@cpojer - @davidaurelio I had a great meeting with @matthargett @lelandrichardson and @rozele

In the meeting we decided the following: There are benefits in all platforms to add this feature. Sketch/Web/Xbox/Windows etc. It is essential that we provide platform needs and attempt to remove any platform specific code from RN core.

Proposed Solution

A function in package.json would list the platform + cascade.

package.json is chosen over rn-cli.config.js because this feature will be useful in web. Web - via webpack, doesn’t currently care/understand the rn-cli.config.js.

For RN, platforms paths are hardcoded, there will be a value called getProvidesModuleNodeModules which will be exposed in rn-cli.config.js which will allow node-haste to traverse additional paths, like it does in defaults.js here: https://github.com/facebook/react-native/blob/72157cf99164d00c7a14b6b9ca51b406080b5265/packager/defaults.js#L36-L39

When package.json has the given values, packager will simply check if a cascade was provided, if so, use the array, if not, use the default.

BONUS POINTS - Any package.json setting of a included platform lib will be merged as well. This simplifies the experience for platform devs to not have to write to each project's package.json. ergo node_modules/windows/package.json can be used instead of the project one, but the project is always final say.

Next Steps

andrewimm commented 7 years ago

@GantMan heads up, I have a FB-internal PR to already add the getProvidesModuleNodeModules. It got slowed by the holiday season, but should be pushed out to the public master soon.

mkonicek commented 7 years ago

getProvidesModuleNodeModules - that's a long and hard to read name, why not call this something like getHasteModules?

andrewimm commented 7 years ago

It's definitely awkward. Since it refers to the packages in node_modules that should be able to use Haste resolution, getHastePackages or getHasteNodeModules might be a little more accurate. getHasteModules sounds like it references the modules that can be required, but that might just be me.

cpojer commented 7 years ago

Haste packages are different from haste modules.

I would like to keep this name as it is used throughout the codebase already and a shorter name will introduce confusion. It's also not something that everyone will be exposed to.

We may get rid of this entirely in the future and we may introduce a new option to replace it that makes more sense in the future.

hramos commented 7 years ago

Closing this issue because it has been inactive for a while. If you think it should still be opened let us know why.

davidaurelio commented 7 years ago

I think we might eventually want to pick this one up again

hramos commented 7 years ago

Hi there! This issue is being closed because it has been inactive for a while. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. Either way, we're automatically closing issues after a period of inactivity. Please do not take it personally!

If you think this issue should definitely remain open, please let us know. The following information is helpful when it comes to determining if the issue should be re-opened:

If you would like to work on a patch to fix the issue, contributions are very welcome! Read through the contribution guide, and feel free to hop into #react-native if you need help planning your contribution.