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: fuels dev hangs after compilation errors #3504

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
a34e804
fix: `fuels dev` hangs after compilation errors
Dhaiwat10 Dec 26, 2024
e281623
revert debug changes
Dhaiwat10 Dec 26, 2024
4034899
reset files
Dhaiwat10 Dec 26, 2024
3851e85
reset
Dhaiwat10 Dec 26, 2024
a1dac4b
rename var
Dhaiwat10 Dec 26, 2024
4ce66e6
fix test
Dhaiwat10 Dec 26, 2024
c5c693f
add changeset
Dhaiwat10 Dec 26, 2024
4319dd1
fix typo
Dhaiwat10 Dec 27, 2024
0d1e614
Merge remote-tracking branch 'origin/master' into dp/fuels-dev-hang
nedsalk Jan 2, 2025
b7270c3
Merge remote-tracking branch 'origin/master' into dp/fuels-dev-hang
nedsalk Jan 7, 2025
3a376f3
wip
nedsalk Jan 7, 2025
1054d91
Merge remote-tracking branch 'origin/master' into dp/fuels-dev-hang
nedsalk Jan 7, 2025
a84fb39
Merge remote-tracking branch 'origin/master' into dp/fuels-dev-hang
nedsalk Jan 9, 2025
81dceca
refactor into file
nedsalk Jan 9, 2025
91b6a95
fix tests
nedsalk Jan 9, 2025
a918ab7
convert to `execSync`
nedsalk Jan 9, 2025
355b882
prefer `rmdirSync`
nedsalk Jan 9, 2025
8319906
simplify try/catch
nedsalk Jan 9, 2025
f702c91
fix lint
nedsalk Jan 9, 2025
5367dce
Revert "fix lint"
nedsalk Jan 9, 2025
3ed5ff1
Reapply "fix lint"
nedsalk Jan 9, 2025
9e8d1e5
Merge remote-tracking branch 'origin/master' into dp/fuels-dev-hang
nedsalk Jan 10, 2025
8a9eee9
Update packages/fuels/src/cli/commands/withConfig.test.ts
nedsalk Jan 10, 2025
5da0b2f
Update .changeset/silly-pianos-fry.md
nedsalk Jan 10, 2025
63ac164
Update packages/fuels/src/cli/commands/withConfig.test.ts
nedsalk Jan 10, 2025
dd84b63
Merge remote-tracking branch 'origin/master' into dp/fuels-dev-hang
nedsalk Jan 13, 2025
7c333c9
make `withConfig` async again
nedsalk Jan 13, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/silly-pianos-fry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"fuels": patch
---

fix: `fuels dev` hangs after compilation errors
9 changes: 2 additions & 7 deletions packages/fuels/src/bin.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
import { error } from './cli/utils/logger';
import { run } from './run';

try {
run(process.argv).catch((x) => {
arboleya marked this conversation as resolved.
Show resolved Hide resolved
// eslint-disable-next-line no-console
console.log(x);
});
} catch (err: unknown) {
run(process.argv).catch((err) => {
error((err as Error)?.message || err);
process.exit(1);
}
});
3 changes: 1 addition & 2 deletions packages/fuels/src/cli/commands/build/forcHandlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ type OnErrorFn = (reason?: number | Error) => void;
export const onForcExit =
(onResultFn: OnResultFn, onErrorFn: OnErrorFn) => (code: number | null) => {
if (code) {
onErrorFn(code);
// process.exit()?
onErrorFn(new Error(`forc exited with error code ${code}`));
Copy link
Contributor

@nedsalk nedsalk Jan 9, 2025

Choose a reason for hiding this comment

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

The onErrorFn would be a Promise's reject callback for a failed forc build spawn, and previously it'd reject with 1, which isn't possible to re-throw in the withConfigErrorHandler as I changed to be done now.

} else {
onResultFn();
}
Expand Down
2 changes: 1 addition & 1 deletion packages/fuels/src/cli/commands/dev/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ describe('dev', () => {

const withConfigErrorHandler = vi
.spyOn(withConfigMod, 'withConfigErrorHandler')
.mockReturnValue(Promise.resolve());
.mockReturnValue(undefined as never);

const loadConfig = vi
.spyOn(loadConfigMod, 'loadConfig')
Expand Down
2 changes: 1 addition & 1 deletion packages/fuels/src/cli/commands/dev/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export const configFileChanged = (state: DevState) => async (_event: string, pat
// eslint-disable-next-line @typescript-eslint/no-use-before-define
await dev(await loadConfig(state.config.basePath));
} catch (err: unknown) {
await withConfigErrorHandler(<Error>err, state.config);
withConfigErrorHandler(<Error>err, state.config);
}
};

Expand Down
2 changes: 1 addition & 1 deletion packages/fuels/src/cli/commands/node/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ describe('node', () => {

const withConfigErrorHandler = vi
.spyOn(withConfigMod, 'withConfigErrorHandler')
.mockReturnValue(Promise.resolve());
.mockReturnValue(undefined as never);
arboleya marked this conversation as resolved.
Show resolved Hide resolved

const loadConfig = vi
.spyOn(loadConfigMod, 'loadConfig')
Expand Down
4 changes: 2 additions & 2 deletions packages/fuels/src/cli/commands/withConfig.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ describe('withConfig', () => {
shouldErrorOnDeploy: true,
});

await withConfig(command, Commands.deploy, deploy)();
await expect(withConfig(command, Commands.deploy, deploy)).rejects.toThrow();

expect(loadConfig).toHaveBeenCalledTimes(1);
expect(loadConfig.mock.calls[0][0]).toEqual(configPath);
Expand All @@ -84,7 +84,7 @@ describe('withConfig', () => {
shouldErrorOnLoadConfig: true,
});

await withConfig(command, Commands.deploy, deploy)();
await expect(withConfig(command, Commands.deploy, deploy)).rejects.toThrow();

expect(loadConfig).toHaveBeenCalledTimes(1);
expect(loadConfig.mock.calls[0][0]).toEqual(configPath);
Expand Down
11 changes: 5 additions & 6 deletions packages/fuels/src/cli/commands/withConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,10 @@ import { loadConfig } from '../config/loadConfig';
import type { Commands, FuelsConfig, CommandEvent } from '../types';
import { error, log } from '../utils/logger';

export const withConfigErrorHandler = async (err: Error, config?: FuelsConfig) => {
export const withConfigErrorHandler = (err: Error, config?: FuelsConfig) => {
error(err.message);
if (config) {
await config.onFailure?.(config, <Error>err);
Copy link
Member

Choose a reason for hiding this comment

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

Why go from async to sync?

Copy link
Contributor

@nedsalk nedsalk Jan 13, 2025

Choose a reason for hiding this comment

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

It was always sync because onFailure returns void. ref

We could change the signature to be Promise<void> | void in some other PR - it comes in handy to be able to pass async callbacks.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. I think we could then fix the return type (still in this PR) and keep it async because the user may have an async tear-down routine for these failure scenarios, and if we don't wait, things may be left dirty.

If we leave types aside for one second, we'd be doing something similar to what we did with the test utilities for asserting Fuel errors, although this case is much less convoluted. By considering it could always be async and always awaiting for it, we cover both cases.

Copy link
Contributor

@nedsalk nedsalk Jan 13, 2025

Choose a reason for hiding this comment

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

I have reverted withConfigErrorHandler to be async again in 7c333c9, as well as all of the other related changes like this await, and have created #3582 because all our event listeners are synchronous. I have included this onFailure listener there as well. I think it's better to cover it all in one PR for that issue. Sounds good?

}
config?.onFailure?.(config, <Error>err);
throw err;
};

export function withConfig<CType extends Commands>(
Expand All @@ -28,15 +27,15 @@ export function withConfig<CType extends Commands>(
try {
config = await loadConfig(options.path);
} catch (err) {
await withConfigErrorHandler(<Error>err);
withConfigErrorHandler(<Error>err);
return;
}

try {
await fn(config, program);
log(`🎉 ${capitalizeString(command)} completed successfully!`);
} catch (err: unknown) {
await withConfigErrorHandler(<Error>err, config);
withConfigErrorHandler(<Error>err, config);
}
};
}
20 changes: 20 additions & 0 deletions packages/fuels/test/features/dev.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { deferPromise } from '@fuel-ts/account';
import { spawn } from 'child_process';
import * as chokidar from 'chokidar';
import { readFileSync, writeFileSync } from 'node:fs';
import { join } from 'path';
import { cwd } from 'process';

Expand All @@ -11,6 +12,7 @@ import { mockCheckForUpdates } from '../utils/mockCheckForUpdates';
import { mockLogger } from '../utils/mockLogger';
import { resetDiskAndMocks } from '../utils/resetDiskAndMocks';
import { runInit, runDev, bootstrapProject, resetConfigAndMocks } from '../utils/runCommands';
import { runInitTemp } from '../utils/testHelpers';

vi.mock('chokidar', async () => {
const mod = await vi.importActual('chokidar');
Expand Down Expand Up @@ -82,6 +84,24 @@ describe('dev', () => {
expect(on).toHaveBeenCalledTimes(2);
});

it('exits when build fails', { timeout: 30_000 }, async () => {
using temp = runInitTemp();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an example test that benefits from the idea explained in

const mainSw = readFileSync(`${temp.contractDir}/src/main.sw`).toString();
const invalidSwayCode = `${mainSw}\nabi `;
writeFileSync(`${temp.contractDir}/src/main.sw`, invalidSwayCode);

const devProcess = spawn('pnpm fuels dev', {
cwd: temp.rootDir,
detached: true,
shell: 'bash',
});

await new Promise((resolve) => {
devProcess.on('exit', () => {
resolve(undefined);
});
});
});
test('`dev` command can work with ephemeral port 0', { timeout: 25000 }, async () => {
await runInit({
root: paths.root,
Expand Down
34 changes: 34 additions & 0 deletions packages/fuels/test/utils/testHelpers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { execSync } from 'child_process';
import { randomUUID } from 'crypto';
import { mkdirSync, rmdirSync } from 'fs';
import { tmpdir } from 'os';
import path from 'path';

export function runInitTemp() {
const fuelsPath = path.join(process.cwd(), 'packages/fuels');

const rootDir = path.join(tmpdir(), '.fuels', 'tests', randomUUID());

mkdirSync(rootDir, { recursive: true });

execSync('pnpm init', { cwd: rootDir });
execSync(`pnpm link ${fuelsPath}`, { cwd: rootDir });
Dismissed Show dismissed Hide dismissed

const contractDir = path.join(rootDir, 'contract');
const outputDir = path.join(rootDir, 'output');
mkdirSync(contractDir);
mkdirSync(outputDir);

execSync(`${process.env.FORC_PATH} init`, { cwd: contractDir });
execSync(`pnpm fuels init -o ${outputDir} -c ${contractDir} --fuel-core-port 0`, {
cwd: rootDir,
});

return {
rootDir,
contractDir,
[Symbol.dispose]: () => {
rmdirSync(rootDir, { recursive: true });
},
};
}
3 changes: 0 additions & 3 deletions vitest.global-setup.ts

This file was deleted.

7 changes: 5 additions & 2 deletions vitest.shared.config.mts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ export default defineConfig({
],
esbuild: { target: "es2022" },
test: {
globalSetup: ["vitest.global-setup.ts"],
setupFiles: ["./vitest.setup-files.ts"],
coverage: {
enabled: true,
Expand Down Expand Up @@ -42,7 +41,11 @@ export default defineConfig({
"/apps/demo-react-vite",
],
globals: true,
env: loadEnv(mode, process.cwd(), ""),
env: {
...loadEnv(mode, process.cwd(), ""),
FUEL_CORE_PATH: "fuels-core",
FORC_PATH: "fuels-forc",
},
poolOptions: {
threads: {
minThreads: 1,
Expand Down
Loading