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

"Breaking - remove unused registration of JS modules" really breaks stuff #15232

Closed
forki opened this issue Jul 27, 2017 · 18 comments
Closed

"Breaking - remove unused registration of JS modules" really breaks stuff #15232

forki opened this issue Jul 27, 2017 · 18 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@forki
Copy link
Contributor

forki commented Jul 27, 2017

Is this a bug report?

no. but as already discussd below this can lead to mayn bug reports in downstream projects

Have you read the Bugs section of the How to Contribute guide?

yes

Environment

  1. react-native -v: v0.47.0-rc.5
  2. node -v: v7.0.0
  3. npm -v: 3.10.9
  • Target Platform: Android
  • Development Operating System: Win 10
  • Build tools: command line

Steps to Reproduce

Create a RN 0.47rc-5 project that uses https://github.com/ideacreation/react-native-barcodescanner or one of many other 3rd-party libs. Try to compile and run.

Expected Behavior

App will run as in 0.46

Actual Behavior

Compile error.

D:\Work\835a948d\0\products\msu.Reading\node_modules\react-native-barcodescanner\android\src\main\java\com\eguma\barcodescanner\BarcodeScannerPackage.java:20: error: method does not override or implement a method from a supertype
    @Override
    ^
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
1 error
:react-native-barcodescanner:compileReleaseJavaWithJavac FAILED

FAILURE: Build failed with an exception.

Background

In ce6fb33 @javache removed the JS registration because it's now unnecessary. The issue is that a lot of the RN third-party components now need recompilation or just break (see ideacreation/react-native-barcodescanner#79 for one example but quick google search shows more).
That alone is not a problem but some components didn't have updates dor a long time and maybe authors are hard to reach.

The question is: Is this really needed?
What if we keep the abstract method is still there and just doesn't call anymore. Then components wouldn't need to recompile, right? This could save a lot of trouble in the ecosystem.

@hramos
Copy link
Contributor

hramos commented Jul 27, 2017

I'm not sure this is the best place to handle this. Closing as the issue did not follow the template.

@hramos hramos closed this as completed Jul 27, 2017
@forki
Copy link
Contributor Author

forki commented Jul 27, 2017

Where do you think I should open such a question?

@hramos
Copy link
Contributor

hramos commented Jul 27, 2017

We're focusing solely on bugs here in the issue tracker. There's several places listed in the contribution docs where you can ask questions: http://facebook.github.io/react-native/docs/contributing.html#how-to-get-in-touch

@forki
Copy link
Contributor Author

forki commented Jul 27, 2017 via email

@hramos
Copy link
Contributor

hramos commented Jul 27, 2017

Alright. Can you edit your original post and clarify that you're proposing a change? Ideally, follow the template as close as possible so it's clear for the maintainers what you're proposing, etc.

@hramos hramos reopened this Jul 27, 2017
@forki
Copy link
Contributor Author

forki commented Jul 28, 2017

done. thanks for reopening

@javache
Copy link
Member

javache commented Jul 28, 2017

If you have any suggestions for making this change more incremental, I'm all ears. Since ReactPackage is an interface, we can't have an optional method there, so getting rid of it as a non-breaking change seems impossible.

Even if we keep it in, but don't call it, it will be a breaking change at some point if we want to clean this interface.

@forki
Copy link
Contributor Author

forki commented Jul 28, 2017

yes if you need to clean the interface, then it will be breaking. That's for sure.
But there is no need as far as I can see. Everything will continue to work, right?

While I agree from a OCD standpoint it's a smell to keep that abstract method. But if you don't want to break the ecosystem then it probably makes a lot of sense to use @Deprecated for a while (I think that is the one in java, right?)

@javache
Copy link
Member

javache commented Jul 28, 2017

Adding @deprecated will still force new package developers to implement this method, even if it's totally useless.

We really try to limit these types of breaking changes as much as possible, but we also don't want to bog down the framework with technical debt and legacy API's.

@forki
Copy link
Contributor Author

forki commented Jul 28, 2017

next instance: react-native-image-picker/react-native-image-picker#649

It hits everyone.

@forki
Copy link
Contributor Author

forki commented Jul 28, 2017

next instance: bamlab/react-native-image-resizer#92

@forki
Copy link
Contributor Author

forki commented Jul 28, 2017

next instance: OneSignal/react-native-onesignal#292

@lukewlms
Copy link

lukewlms commented Aug 2, 2017

For maintainers of those packages: looks like the best change is just to remove the @OverRide line, while still implementing the function.

That will ensure compatibility with both 0.46.x and 0.47.x.

See e.g. OneSignal/react-native-onesignal#294

Also see react-native-code-push which removed the whole function, then was forced to release a new version that was broken on 0.46.x.

@radko93
Copy link
Contributor

radko93 commented Aug 2, 2017

I've tested on RN 0.46.4 with this released and it works.

@hramos
Copy link
Contributor

hramos commented Aug 3, 2017

The change is intentional, and doing this in a non-breaking manner does not appear to be possible in a clean manner. A reasonable workaround has been provided. Closing.

@ujwal-setlur
Copy link

It is unfortunate that this has to be done in a breaking manner.A lot of third party packages are affected. I understand the technical issue, and while I don't want to come across whining, it does seem that every release of react-native has some issue or another that prevents upgrading. 0.46 for example was unbuildable on iOS devices in the first release. Now this. It does create quite a bit of friction in the developer experience unfortunately :-(

These days, I dread doing a react-native upgrade...

@hramos
Copy link
Contributor

hramos commented Aug 4, 2017

We're working on making this better. Several things have happened over the past couple of months that have led to this:

  • We haven't had enough people using the release candidate versions of React Native. As a result, show-stopper issues are not always found until a stable branch is cut and released to everyone.
  • The switch to the React 16 alpha broke Enzyme, which prevented some of our larger and most vocal core contributors from upgrading to the last few React Native release candidates, which leads back to my previous point on surfacing serious issues.
  • Finally, we've had a serious case of issues going out of control. For the last few months, the number of open issues has remained in the ~1,200 range. We've found that some of these show stoppers were indeed reported, but they're quickly drowned out by the other 100 issues that get opened each week. Thankfully we're now down to 600 open issues, and being stricter about following the template has helped us slow down the number of new issues that get opened without sufficient information.

There's a few ways you can help us here: use the release candidates! Upgrade to the RC and let us know ASAP if you run into any issues. When opening an issue, make sure to follow the template, and make note that the issue is a show-stopper that needs to be addressed before the next stable release is cut.

If you're not able to use the release candidates, you can also help us by triaging open issues and flagging those that need to be looked at before a stable branch is cut.

This last point helps more than you might think. We use React Native from master here at Facebook, and issues that affect our own apps are addressed quickly. By helping us triage open issues, you may discover something that is already fixed in master but needs to be cherry-picked into the release candidate prior to the branch cut.

@ujwal-setlur
Copy link

ujwal-setlur commented Aug 4, 2017

@hramos, I understand the challenges. I really love react-native and want to see it get even more awesome. Some thoughts:

  1. I think you guys need to have a more formal ecosystem partnership with third party package providers so that they understand what's coming down the pipe. A lot of packages are slow to catch up to changes to react-native interfaces. For example, your own react-native-fbsdk oAuth module itself had not caught up with this change. I had to pull from master. If this mechanism is already in place, it needs to get better.

  2. Some packages seem just abandoned, things that are quite critical to a modern app. This is the dark side of open source. A proliferation of forks come about because the package maintainer is MIA. I myself have forked a few because I needed critical fixes. I don't know what the solution to this is, but maybe Facebook can take more ownership of some critical native modules.

  3. All the issues that I found, Facebook should have found too given that you consume this for your apps. I have been been regularly upgrading to official releases, but have stayed away from rc-releases. I am building a production app and am nervous about switching to non official releases, but I am willing to try on some of myth other projects.

All in all, I want my developer experience to improve, and I am willing to help. I now have modified my build system by creating a script that patches third party boo-boos post npm install :)

YauhenBichel added a commit to YauhenBichel/react-native-bottomsheet that referenced this issue Aug 7, 2017
Method createJSModules was removed from RN 0.47. Which leads to compilation error due to @OverRide annotation. This method can be removed right now (facebook/react-native#15232)
YauhenBichel added a commit to YauhenBichel/react-native-linear-gradient that referenced this issue Aug 7, 2017
Method createJSModules was removed from RN 0.47. Which leads to compilation error due to @OverRide annotation. This method can be removed right now (facebook/react-native#15232)
treavis added a commit to treavis/react-native-rabbitmq that referenced this issue Sep 5, 2017
yelworc added a commit to uniqlabs/react-native-audio-streamer that referenced this issue May 16, 2018
Slightly adjust the Android package according to a breaking change in
RN 0.47. Just removing the `@Override` annotation is the suggested
backwards-compatible fix; see:
facebook/react-native#15232 (comment)
@facebook facebook locked as resolved and limited conversation to collaborators Aug 3, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Aug 3, 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
@javache @forki @hramos @ujwal-setlur @radko93 @lukewlms @react-native-bot and others