-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: fuels dev
hangs after compilation errors
#3504
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
This PR is published in NPM with version 0.0.0-pr-3504-20241227154428 |
CodSpeed Performance ReportMerging #3504 will degrade performances by 48.39%Comparing Summary
Benchmarks breakdown
|
// because forc logs out 'Aborting due to 2 compilation errors' etc. | ||
onForcError(reject); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried compiling an invalid project with forc build
and it exited with an error code 1
, so it seems to me that we shouldn't be parsing forc
logs but rather just throwing if forc build
exits with a non-zero code. Line 37: forc.on('error', onError)
should've caught this but it didn't 🤔 Did you investigate this avenue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
forc.on('error, onError)
doesn't do anything because forc
sends error messages to stdErr
alongside other normal logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May it be worthwhile moving this "Abort listener" to outside of the loggingConfig.isLoggingEnabled
, after the forc.on('error', onError);
on line 37?
Coverage Report:
Changed Files:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work @Dhaiwat10!
I found while testing, that if the file has a compilation error on start-up, the watchers never get registered. Did you encounter anything similar?
// because forc logs out 'Aborting due to 2 compilation errors' etc. | ||
onForcError(reject); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May it be worthwhile moving this "Abort listener" to outside of the loggingConfig.isLoggingEnabled
, after the forc.on('error', onError);
on line 37?
@@ -138,8 +138,6 @@ export async function deployContracts(config: FuelsConfig) { | |||
|
|||
const wallet = await createWallet(config.providerUrl, config.privateKey); | |||
|
|||
log(`Deploying contracts to: ${wallet.provider.url}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to remove this log?
Not to my knowledge, but I haven't tested that scenario either. |
fuels dev
hangs on generating types #1886Release notes
In this release, we:
fuels dev
would hang after a Sway compilation errorSummary
This PR fixes the issue where
fuels dev
would hang after a Sway compilation error.To test:
pnpm create fuels@pr-3504
pnpm fuels:dev
andpnpm dev
Checklist