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.

Sign up for GitHub

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

[Typechain] generate types only for contracts and not test files #6400

Open
wants to merge 4 commits into
base: v-next
Choose a base branch
from
Open

[Typechain] generate types only for contracts and not test files #6400

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
02f2980
remove old hook and add a new one
ChristopherDedominici Feb 24, 2025
739f6b3
Create calm-clouds-work.md
ChristopherDedominici Feb 24, 2025
bf8df70
Merge branch 'v-next' into typechain-do-not-build-test-files
ChristopherDedominici Mar 4, 2025
e5d48d1
Merge branch 'v-next' into typechain-do-not-build-test-files
kanej Mar 4, 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
15 changes: 5 additions & 10 deletions v-next/hardhat-typechain/src/internal/hook-handlers/solidity.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,25 @@
import type { HookContext, SolidityHooks } from "hardhat/types/hooks";
import type { CompilationJob } from "hardhat/types/solidity";

import { generateTypes } from "../generate-types.js";

export default async (): Promise<Partial<SolidityHooks>> => {
const handlers: Partial<SolidityHooks> = {
async onAllArtifactsEmitted(
async onCleanUpArtifacts(
context: HookContext,
artifacts: Map<CompilationJob, ReadonlyMap<string, string[]>>,
artifactPaths: string[],
next: (
nextContext: HookContext,
artifacts: Map<CompilationJob, ReadonlyMap<string, string[]>>,
artifactPaths: string[],
) => Promise<void>,
) {
const artifactsPaths = Array.from(artifacts.values()).flatMap(
(innerMap) => Array.from(innerMap.values()).flat(),
);

await generateTypes(
context.config.paths.root,
context.config.typechain,
context.globalOptions.noTypechain,
artifactsPaths,
artifactPaths,
);

return next(context, artifacts);
return next(context, artifactPaths);
},
};

Expand Down
14 changes: 7 additions & 7 deletions v-next/hardhat/src/internal/builtin-plugins/solidity/build-system/build-system.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,13 +211,6 @@ export class SolidityBuildSystemImplementation implements SolidityBuildSystem {
);
}),
);

await this.#hooks.runHandlerChain(
"solidity",
"onAllArtifactsEmitted",
[contractArtifactsGeneratedByCompilationJob],
async () => {},
);
}

const resultsMap: Map<string, FileBuildResult> = new Map();
Expand Down Expand Up @@ -645,6 +638,13 @@ export class SolidityBuildSystemImplementation implements SolidityBuildSystem {
duplicatedContractNamesDeclarationFilePath,
getDuplicatedContractNamesDeclarationFile(duplicatedNames),
);

await this.#hooks.runHandlerChain(
Copy link
Contributor Author

@ChristopherDedominici ChristopherDedominici Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Pato mentioned that probably this is the best place to add a hook that can emit the artifacts paths.
His message:
I think that we should change the moment when we generate the typechain types. Its now on build, but i believe it should be in cleanArtifacts. That already generates some typescript types for the artifacts. Especially when there are clashes between names. This would mean removing the hook, creating a new one after cleanupArtifacts, and using that one instead.

Copy link
Member

Choose a reason for hiding this comment

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

This would mean we generate types only when the entire project is compiled - see

hardhat/v-next/hardhat/src/internal/builtin-plugins/solidity/tasks/compile.ts

Line 37 in 4554b50

// If we recompiled the entire project we cleanup the artifacts
- which, as far as I understand, is not necessarily what we want.

To be fair, I also find hooking types generation to the cleanup step a little confusing. It is not obvious to know what the type generation has to do with cleanup without deeply understanding the internals. I think there might be some potential for the refactor in the cleanup function, but that's definitely out of the scope of this PR.

However, as far as when the type generation should get triggered, maybe it should be when the artifacts are emitted after all. But maybe we could add some option to the build and consequently emitArtifacts to control whether artifact declaration files -

hardhat/v-next/hardhat/src/internal/builtin-plugins/solidity/build-system/build-system.ts

Line 527 in 4554b50

const artifactsDeclarationFilePath = path.join(
- are emitted. Then, we could either propagate that option via the hook system or check whether the declaration file exists for a given artifact on the typechain side. There definitely might be better options, just thinking out loud.

"solidity",
"onCleanUpArtifacts",
[artifactPaths],
async () => {},
);
}

public async compileBuildInfo(
Expand Down
14 changes: 7 additions & 7 deletions v-next/hardhat/src/internal/builtin-plugins/solidity/type-extensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import type { SolidityBuildSystem } from "../../../types/solidity/build-system.js";
import type { CompilationJob } from "../../../types/solidity.js";

import "../../../types/config.js";
declare module "../../../types/config.js" {
Expand Down Expand Up @@ -99,19 +98,20 @@ declare module "../../../types/hooks.js" {

export interface SolidityHooks {
/**
* Provide a handler for this hook to retrieve all artifacts created by a compilation job.
Copy link
Contributor Author

@ChristopherDedominici ChristopherDedominici Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The old hook can be removed in favor of the new one. See previous comment for more details

* Hook triggered during the cleanup process of Solidity compilation artifacts.
* This hook runs after unused artifacts and build-info files have been removed.
*
* @param context The hook context.
* @param artifacts A map of the artifacts created by each compilation job.
* @param artifactPaths The file paths of artifacts that remain after cleanup.
* @param next A function to call the next handler for this hook, or the
* default implementation if there are no more handlers.
* default implementation if no more handlers exist.
*/
onAllArtifactsEmitted: (
onCleanUpArtifacts: (
context: HookContext,
artifacts: Map<CompilationJob, ReadonlyMap<string, string[]>>,
artifactPaths: string[],
next: (
nextContext: HookContext,
artifacts: Map<CompilationJob, ReadonlyMap<string, string[]>>,
artifactPaths: string[],
) => Promise<void>,
) => Promise<void>;
}
Expand Down
Loading