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

Fixed getChainIdDefault() race condition #1648

Merged
merged 2 commits into from
May 17, 2023

Conversation

jribbink
Copy link
Contributor

Closes #1641

@jribbink jribbink requested a review from a team as a code owner May 12, 2023 22:59
@changeset-bot
Copy link

changeset-bot bot commented May 12, 2023

🦋 Changeset detected

Latest commit: 3c39de9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@onflow/fcl Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@jribbink jribbink force-pushed the jribbink/chainid-race-condition branch from ff92ca3 to 17ee594 Compare May 12, 2023 23:01
@chasefleming chasefleming mentioned this pull request May 15, 2023
7 tasks
Copy link
Member

@chasefleming chasefleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@chasefleming chasefleming self-requested a review May 15, 2023 22:39
Copy link
Member

@chasefleming chasefleming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jribbink Can we add a test for this?

@jribbink
Copy link
Contributor Author

@jribbink Can we add a test for this?

yeah, my bad

Copy link
Member

@JeffreyDoyle JeffreyDoyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just needs a test(s)!

@bluesign
Copy link

Thanks, this was causing also problems with some bundlers

@jribbink jribbink force-pushed the jribbink/chainid-race-condition branch from 1c68096 to 2f554b3 Compare May 16, 2023 21:35
@jribbink
Copy link
Contributor Author

jribbink commented May 16, 2023

Sorry, added the tests now. I had to break the watcher function out into a separate file to write tests with mocks. Also just renamed getChainId.js since it broke JS file naming conventions and was bothering me 😛

@jribbink jribbink force-pushed the jribbink/chainid-race-condition branch 2 times, most recently from 7225901 to 1530845 Compare May 16, 2023 21:52
import {setChainIdDefault} from "./get-chain-id"

/**
* @description
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding JSDoc!

@jribbink jribbink dismissed JeffreyDoyle’s stale review May 17, 2023 21:12

stale review, tests have been added

@jribbink jribbink merged commit 06655ae into master May 17, 2023
@jribbink jribbink deleted the jribbink/chainid-race-condition branch May 17, 2023 21:13
@github-actions github-actions bot mentioned this pull request Jun 2, 2023
@chasefleming chasefleming mentioned this pull request Jun 2, 2023
5 tasks
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

Successfully merging this pull request may close these issues.

[BUG] config() race condition related to top-levelsetChainIdDefault() call
4 participants