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

[3.0.0] Chrome Debugging does not work if you use Sync APIs / Invariant violation: Calling synchronous methods on native modules is not supported in Chrome #776

Closed
Johan-dutoit opened this issue Sep 11, 2019 · 79 comments

Comments

@Johan-dutoit
Copy link
Collaborator

As per the title, I get this when debugging on an iOS simulator (yet to try android, but assume it would be the same).

image

@mikehardy
Copy link
Collaborator

Dang - and the reason we went away from constants is because app init was taking a long time (c.f. getUserAgent). This is a problem that needs thought.

I don't personally use a debugger (strange but true) thus had not noticed at all, and this doesn't affect me (your current maintainer) but I recognize it's important to many.

I'm open to any suggestions.

@mikehardy
Copy link
Collaborator

I will say that I'm happy to release a breaking 4.0 or whatever if a solution is proposed that is workable for the synchronous use case. Perhaps the compromise is that only certain items are exposed synchronously, are fetched as constants, and the rest is async only. Then we just categorize which APIs get the constants + sync API treatment with sensitivity to startup time and it's good enough?

@mikehardy
Copy link
Collaborator

In chatting with @Johan-dutoit it appears this only happens if you use the sync APIs - if your app is async-only APIs then chrome devtools should be fine?

I will add a mention of this to the docs, and that makes me think that the compromise strategy of loading a few things as constants underneath the sync APIs and having the rest (the not likely to be used at app startup items) be "dangerous" might be the best we can do to balance my desire to support sync use case at startup, and debugging, and fast init time.

@mikehardy mikehardy changed the title [3.0.0] Invariant violation: Calling synchronous methods on native modules is not supported in Chrome [3.0.0] Chrome Debugging does not work if you use Sync APIs / Invariant violation: Calling synchronous methods on native modules is not supported in Chrome Sep 11, 2019
@mikehardy mikehardy pinned this issue Sep 11, 2019
@MakhouT
Copy link

MakhouT commented Sep 18, 2019

Having the same issue. Don't really know how to help as I really don't know what's happening.

@mikehardy
Copy link
Collaborator

@MakhouT if you use the debugger, and you touch a xxxSync() method, the debugger will fail. If you want to use the debugger currently, you must not use any xxxSync() methods. That means you need to convert to a fully async style and if there are items you need at startup then before you create your App and display your root view, you need to fetch those items during bootstrap and the app creation / root view display needs to be in a .then() after you've got the items you need.

I recognize this is painful - being fully async on critical items during bootstrap - which is why Sync() APIs exist but they blow up the debugger, which is why I propose a pragmatic solution of making a few select items available via constants (which are synchronous) again but I haven't made that PR yet.

That's all the info I've got. Hope it helps.

@TimMun
Copy link

TimMun commented Sep 20, 2019

FWIW this is my workaround in my app, basically following the above comments. In short, when my app's root component mounts, I throw up a loading screen while I populate my needed constants, then when that is finished I render the rest of my app.

First I create a deviceInfo.js:

// deviceInfo.js
import DeviceInfo from 'react-native-device-info'

var output = {}

exports.populateDeviceInfo = async () => {
  output.isEmulator = await DeviceInfo.isEmulator()
  output.version = await DeviceInfo.getVersion()
 // ... and whatever else you may need
}

export default output

Then in my AppContainer.js (my root component). I have some stuff relating to redux below but obv adjust it to whatever your app views should be.

// (import your components, etc)
import { populateDeviceInfo } from './deviceInfo'

export default class AppContainer extends Component {

  state = {
    deviceInfoLoaded: false
  }

  componentDidMount = async () => {
    await populateDeviceInfo()
    this.setState({ deviceInfoLoaded: true })
  }

  render() {
    return (
      <Provider store={store}>
        <PersistGate loading={<LoadingView />} persistor={persistor}>
          { this.state.deviceInfoLoaded ? <App /> : <LoadingView /> }
        </PersistGate>
      </Provider>
    )
  }
}

And then afterwards I can just import DeviceInfo from './deviceInfo' where I need it and use the populated constants. I didn't have that many constants to load, nor did they take a huge amount of time, so this worked for me.

@mikehardy
Copy link
Collaborator

That's fantastic, thanks especially for showing working code. That is about exactly what I'd expect to handle a fully async style for things you need before first render, and I'm guessing many (like myself) weren't sure how to handle it so cleanly.

@kirillpisarev
Copy link

kirillpisarev commented Sep 20, 2019

@TimMun Actually there could be the cases when this approach doesn't work. For example when you use the lib within some code that is executed during the app startup. E. g. when you create a StyledComponent object and use isTablet() function inside. So this code won't wait until the populateDeviceInfo object is populated.

@mikehardy
Copy link
Collaborator

I personally had some code that was ill-advised but called DeviceInfo items in the constructor on objects that had a singleton pattern (exported result of MySingleton.getNewInstance() which calls the constructor, which calls DeviceInfo) so just import statements were triggering things. So I do sympathize with the desire for Sync() APIs and for making those work with the debugger.

But in general each example of how to handle things slightly better gets people to consume the API more async which I think is more javascript-idiomatic

@JensDebergh
Copy link
Contributor

JensDebergh commented Sep 25, 2019

@mikehardy

I understand why the API has been rewritten to become async and reduce app boot time but to me it's just weird I need asynchronous calls for constants that are readily available on the device.

Imagine programming in javascript and writing this to fetch a constant:

const MY_CONSTANT_VARIABLE = 1

const fetchMyConstant  = Promise.new(() => {
 resolve(MY_CONSTANT_VARIABLE)
}, reject) 

Writing promises like this for constants is just weird but that's what is essentially happening right now with the rewrite to asynchronous calls.

The sync calls are a great addition but you completely lose debugging functionality for your entire app which breaks every developer team that uses react-native-device-info.

My point is:

All if not most of react-native-device-info calls are short running processes that take milliseconds to complete and send data.

Picking async calls to boost app performance in my opinion isn't the best solution to this problem. I mainly think that sending all this data over the react native bridge is the bottleneck and switching to async calls just pushes the problem down the line while adding extra complexity.

I would rather have a few extra ms of boot time then having to deal with async calls for fetching constants.

If I can help in any way let me know.

@mikehardy
Copy link
Collaborator

mikehardy commented Sep 25, 2019

All if not most of react-native-device-info calls - which ones? for which devices? My first cut was purist and intended to not arbitrarily decide. Just make them all async.

My experience integrating that was that it was irritating at bootstrap yes. So my second cut was to add a companion Sync API.

Only later I learned it blew up debugging. So here we are.

Pragmatically, #784 goes down the arbitrary "let's decide for some calls to not be async" route by restoring a few key things as constants.

For your example I'd say I can imagine writing code like that but why would I do that?

async myUsefulFunction() {
  try {
    const myConstant = await deviceInfoCall();

     // all that other stuff useful functions do
     // seemingly always relying on async stuff anyway so...


  } catch (e) {
    console.log('that sucked');
  }
}

Not trying to debate really. I intend to get have chrome debugging support resolved. If you want to help, work on the draft of that PR - I haven't had time to work on it yet

@JensDebergh
Copy link
Contributor

My example was just to demonstrate react-native-device-info prior to writing most functions from constants to functions.

For example:

These are declared as constants on the module:

Prior to rewrite:

https://github.com/react-native-community/react-native-device-info/blob/master/ios/RNDeviceInfo/RNDeviceInfo.m#L130

Now:

https://github.com/react-native-community/react-native-device-info/blob/41ee8c78e561ff5d689a353095e25c6b7f821218/ios/RNDeviceInfo/RNDeviceInfo.m#L98

I just think stuff like this should be declared as a constant since it's instant and should be accessible as so. Not as a function that should be waited on.

If calling all these methods in succesion is raising boot time significantly maybe we could bundle all these calls and send all info over the bridge in 1 call.

Native:

// EXPORT AS CONSTANT
deviceInfo: {
  systemName: self.systemName,
  bundleId: self.bundleId,
} 

Javascript:

const { systemName, bundleId } = DeviceInfo.getInfo()

// At this point you can cache all values in javascript that won't change like systemName, etc...

// Specific call for up to date battery level
const batteryLevel = await DeviceInfo.getBatteryLevel()

@mikehardy
Copy link
Collaborator

did you look at the linked PR?

@retyui
Copy link

retyui commented Sep 26, 2019

Have this same problem

@thepost
Copy link

thepost commented Sep 26, 2019

100% in agreement with @JensDebergh, very well presented.
#776 (comment)

I come from mainly a native mobile background, and think that async calls for device constants is excessive. It leads to unnecessary complexity, which is something that I'm always against. But if it doesn't impact the user (a delay of a few ms at worst), and especially if there's no clear conclusion if any delay is primarily caused by the bridge or not, it seems wise to just opt for brevity on the API level and use constants instead.


As a solution that this library could offer though, I think it would be good to have something like @TimMun's implementation in react-native-device-info under the hood.

Then just update the docs to advise people to drop in await populateDeviceInfo() in an async componentDidMount at the root level, if they choose to use the async call. Therefore as an interface, calls could remain the same such as getApplicationName etc, only that everyone wouldn't have to create the same helper function to deal with this. 🙂

@mikehardy
Copy link
Collaborator

This is all interesting, we are all in agreement in general, and it does nothing to move the issue forward 🤷‍♂

@JensDebergh
Copy link
Contributor

@mikehardy I'm able to work on the pull request this saturday but I'm only able to the iOS part. My java is terrible.

@mikehardy
Copy link
Collaborator

tag team activate: my ios is terrible but I'm really strong at java :-)

@hellogerard
Copy link

Here I came, just looking to get the updates for iPhone 11's, and walked into a hornet's nest! Programming, am i right?

This is pretty critical. Our app has core, core bootstrap code that has been running for years that would take an entire rewrite to get working with async operations. Using the sync alternatives works, but breaks the debugger, per this issue. I plan on sticking to v2.x of this package and manually working around it for iPhone 11 support. But this won't work forever.

...Here's another temporary solution: we can detect if the debugger is open.

const DEVICE_MODEL = global.__REMOTEDEV__ ? 'Debugger' : DeviceInfo.getModelSync();

I'm using react-native-debugger. YMMV.

@mikehardy
Copy link
Collaborator

I like the workaround - that is potentially useful for people thanks

I don't imagine this will stay unresolved more than another week - this module was already working fine with constants (implication: I/we have code I can refer to quickly to get some constants back without the async or debugger breaking)

Quick question for anyone following along - what is your minimal set of constants you need for bootstrap to work?

There will always be APIs in this module that are async because they are so slow (lookin' at you UserAgent...) so I want to find the middle path that includes the right things as constants but leaves the rest

@hellogerard
Copy link

Here are all the calls we use from this package. It's actually not as many as I had thought.

DeviceInfo.getManufacturer(),
DeviceInfo.getModel(),
DeviceInfo.getSystemVersion(),
DeviceInfo.getBuildNumber(),
DeviceInfo.getUniqueID();
DeviceInfo.getVersion()
DeviceInfo.getBundleId();

@mikehardy
Copy link
Collaborator

those are all included in the ones I was thinking of making constants yes - curious for other responses so I can look at the union of set

@JensDebergh
Copy link
Contributor

JensDebergh commented Sep 26, 2019

@mikehardy Can you post the list you have for properties that are going to be available as constants?

If you have the list of properties I can make the changes this saturday.

@mikehardy
Copy link
Collaborator

4.0.1-rc.1 is out and the migration guide is up, unless bugs are found I will release it tomorrow

https://github.com/react-native-community/react-native-device-info/releases/tag/v4.0.1-rc.1

react-native-community/react-native-device-info/wiki/V3-to-V4-Migration-Guide

Please test it, and edits / improvements to the migration guide are always welcome.

Thanks!

@MakhouT
Copy link

MakhouT commented Oct 13, 2019

I just noticed in the new release we are missing DeviceInfo.getDeviceType(); This is still a Promise currently.

@mikehardy
Copy link
Collaborator

I can see how that would be important for initial layout which is my loose criteria for constants, can you create a fresh issue for it for tracking? I can get it in a 4.0.2 for you as going from Promise to direct return is non-breaking for people I believe (where the other way is not)

@mikehardy
Copy link
Collaborator

@MakhouT you can try the @next tag to get 4.0.2-rc.1 with deviceType as a constant

https://github.com/react-native-community/react-native-device-info/releases/tag/v4.0.2-rc.1

I may need to release it as a breaking change v5 - I think removing the promise is breaking, but you can use it now at least

@mikehardy
Copy link
Collaborator

@MakhouT please report back on 4.0.2-rc.1

@MakhouT
Copy link

MakhouT commented Oct 15, 2019

@mikehardy Thanks for the quick reaction! I haven't had time yesterday, I will try to test it this evening or tomorrow morning. I'll definitely get back to you!

@renatoniro
Copy link

@mikehardy Would it at all be possible to have getDeviceName exposed as a constant? (as in, anything impeding it from being a constant?). I'd imagine it should be pretty safe as (under normal circumstances) the device name shouldn't change between app launches?

@mikehardy
Copy link
Collaborator

getDeviceName is backed by bluetooth on android and can actually change any time, plus going in to that system has costs I'm unsure of but it isn't like a built-in system constant, I'm not sure on that one

@renatoniro
Copy link

Thanks for the explanation @mikehardy - that makes more sense as to why it is implemented that way.

@MakhouT
Copy link

MakhouT commented Oct 19, 2019

@mikehardy Sorry that it took a bit longer than promised. I was able to test it and it works perfectly as expected! Thanks!
Any more changes we can take into V5?

@mikehardy
Copy link
Collaborator

Probably not, but I'm not afraid of major versions, I'm only afraid of accidentally breaking people in a non-major, so no need to wait much. I'll get this packed up and a v5 out probably by Monday

@MakhouT
Copy link

MakhouT commented Oct 19, 2019

Perfect! Thanks!

@acro5piano
Copy link
Contributor

acro5piano commented Oct 24, 2019

I've implemented this hook:

// hooks/useIsEmulator.tsx

import * as React from 'react'
import DeviceInfo from 'react-native-device-info'

export default function useIsEmulator() {
  const [isEmulator, setIsEmulator] = React.useState(false)
  const [loading, setLoading] = React.useState(true)

  React.useEffect(() => {
    DeviceInfo.isEmulator().then(res => {
      setIsEmulator(res)
      setLoading(false)
    })
  }, [])

  return { isEmulator, loading }
}
// App.tsx

import * as React from 'react'
import { Text } from 'react-native'
import useIsEmulator from './hooks/useIsEmulator'

function App() {
  const { isEmulator } = useIsEmulator()
  return <Text>running on {isEmulator ? 'emulator' : 'real device'}</Text>
}

@mikehardy
Copy link
Collaborator

@acro5piano submit a PR please, I can merge it in if it's got all the associated "stuff" (type files, README etc)

@mikehardy
Copy link
Collaborator

good question - device name is not a constant in reality, it may change (e.g., rename your android in bluetooth), the API should reflect that - it is reality

@limaAniceto
Copy link

limaAniceto commented Dec 7, 2020

good question - device name is not a constant in reality, it may change (e.g., rename your android in bluetooth), the API should reflect that - it is reality

Sorry @mikehardy , after re-reading above I noticed you had already replied to that question so I deleted my comment : )

I appreciate you getting back to me so quickly!

Anyway I understand the folks above difficulty as I'm facing it as well. The case is: Styling headers depending on device (while making use of libraries like react-navigation). As you can't add the async wrappers or use state, you can't style the Header and at the same time have a working debugger.

This is definitely not an easy one 😢

@mikehardy
Copy link
Collaborator

For those exact cases we have the Sync calls, but then you can't use the Chrome debugger. It's not an easy thing.
If it really is a fundamental need and you've got no valid workaround it is open source, patch-package makes carrying a local mod pretty easy and you can just add it to constants for your project, knowing that it may change in reality but you're willing to accept it in order for styling to somehow be based on a device name

@AndrinGautschi
Copy link

Hey. I faced this issue today with the getFontScaleSync() method. Is it possible to detect if Chrome Debugging is running and either do this request or replace it with a self defined constant? That would help immensely.

@mikehardy
Copy link
Collaborator

No @AndrinGautschi - I really do not want to gum up the package with chrome debugger-not-chrome-deubugger code. We have sync methods, we have async methods, it's open source, use what you need to use for your use case, patch at will as needed please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests