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

[BUG] config() race condition related to top-levelsetChainIdDefault() call #1641

Closed
jribbink opened this issue May 9, 2023 · 0 comments · Fixed by #1648
Closed

[BUG] config() race condition related to top-levelsetChainIdDefault() call #1641

jribbink opened this issue May 9, 2023 · 0 comments · Fixed by #1648
Assignees
Labels
Bug Something isn't working

Comments

@jribbink
Copy link
Contributor

jribbink commented May 9, 2023

Current Behavior

This "issue" is a bit trivial since it affect end behaviour, but... If you are to run the following code you will notice a bunch of console errors.

import * as fcl from '@onflow/config';
setTimeout(() => {
  fcl.config({
    'accessNode.api': 'https://rest-testnet.onflow.org',
  });
}, 0)
Screen Shot 2023-05-09 at 12 04 44 PM

However, if you run this code, you will notice no errors appear.

import * as fcl from '@onflow/config';
fcl.config({
  'accessNode.api': 'https://rest-testnet.onflow.org',
});

This is because we have a race condition. (i.e. if we are not to set the configuration until the call stack/job queue has emptied, we will see this error). This becomes an issue in something like a react app where we configure fcl in a useEffect hook which is not run immediately.

The specific culprit is this line of code which runs at the top-level (thus right when fcl loads).

setChainIdDefault()

While this bug exists, it does not affect app behaviour because ultimately the chainId is always accessed through getChainId() within fcl & in this function the chain ID is always resolved if it doesn't know it already.

export async function getChainId() {

However, the error can throw developers off, so it is probably best to get rid of.

Expected Behavior

Don't show this error at startup if config hasn't been set 😎.

I suggest one of the following:

  1. Just ditch the top level function (but this will slow down first fcl request)
  2. Do a naive check in setChainIdDefault of whether config.get("accessNode.api") is defined, don't attempt to resolve chainId if it has not been (still imperfect, basically as performant as soln. 1 unless user configures fcl @ top level)
  3. Resolve chainId on accessNode.api change or even just any config change is good enough. (this would be ideal solution)

Steps To Reproduce

Shown above.

Environment

- OS: MacOS Monterey 12.6.5
- Node: v16.18.1
- npm: 8.19.2

What are you currently working on that this is blocking?

Nothing. Bug does not affect application behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants