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

fix: race condition with picked nodes #181

Merged
merged 1 commit into from
Dec 12, 2024
Merged

Conversation

theoephraim
Copy link
Member

fixes an issue with picked item resolution that was introduced when parallelized resolution was introduced. It went unnoticed because the pick tests still resolve correctly because everything was so simple. Introducing a short delay in that test recreates the issue.

Copy link

changeset-bot bot commented Dec 12, 2024

🦋 Changeset detected

Latest commit: 0bbd0fe

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

This PR includes changesets to release 2 packages
Name Type
@dmno/configraph Patch
dmno 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

Copy link

netlify bot commented Dec 12, 2024

Deploy Preview for dmno ready!

Name Link
🔨 Latest commit 0bbd0fe
🔍 Latest deploy log https://app.netlify.com/sites/dmno/deploys/675b7193314a0a0008d58afb
😎 Deploy Preview https://deploy-preview-181--dmno.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 12, 2024

Deploy Preview for signup-api canceled.

Name Link
🔨 Latest commit 0bbd0fe
🔍 Latest deploy log https://app.netlify.com/sites/signup-api/deploys/675b71931c4e1e000820e222

@github-actions github-actions bot added the maintainer Created by an official project maintainer label Dec 12, 2024
Copy link

pkg-pr-new bot commented Dec 12, 2024

Open in Stackblitz

npm i https://pkg.pr.new/@dmno/configraph@181
npm i https://pkg.pr.new/dmno@181
npm i https://pkg.pr.new/@dmno/1password-plugin@181

commit: 0bbd0fe

@@ -15,8 +15,9 @@ export function createdPickedValueResolver(
// since we handle resolution of services in the right order
// we can assume the picked value will be resolved already (if it was possible at all)
if (!sourceNode.isResolved) {
return new Error('picked value has not been resolved yet');
Copy link
Member Author

@theoephraim theoephraim Dec 12, 2024

Choose a reason for hiding this comment

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

it was returning an error as the result, which was getting serialized as [Object object]. I think it went unnoticed because before the parallelization work, the source items were almost always resolved by the time they got picked.

Copy link
Contributor

@philmillman philmillman left a comment

Choose a reason for hiding this comment

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

Nice catch!

@theoephraim theoephraim merged commit 30ef304 into main Dec 12, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer Created by an official project maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants