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

Config loading #5277

Merged
merged 3 commits into from
May 28, 2024
Merged

Config loading #5277

merged 3 commits into from
May 28, 2024

Conversation

zoeyTM
Copy link
Contributor

@zoeyTM zoeyTM commented May 27, 2024

resolves #5263

Copy link

vercel bot commented May 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hardhat ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 28, 2024 7:13am

Copy link

changeset-bot bot commented May 27, 2024

⚠️ No Changeset found

Latest commit: 00397ea

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

* @param projectName The base name of the folder with the project to use.
* @param changeDirTo If provided, the working directory will be changed to this. Must be a child of the project folder.
*/
export function useFixtureProject(projectName: string, changeDirTo?: string) {
Copy link
Member

@schaable schaable May 27, 2024

Choose a reason for hiding this comment

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

@alcuadrado wdyt about having a test module in hardhat-utils for things like this?

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be a separate package, as it should be a devDependency. I'm planning to add it asap. Before we start repeating ourselves.

? configPath
: resolve(process.cwd(), configPath);

const { exists } = await import("@nomicfoundation/hardhat-utils/fs");
Copy link
Member

Choose a reason for hiding this comment

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

We can probably use regular imports for the hardhat-utils package, as the package only imports things from node and should be fast enough to avoid using dynamic imports.

Copy link
Member

Choose a reason for hiding this comment

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

Ignore this comment for now, we'll need more data to decide if and when it is worth using dynamic imports, as most things now will be lazy and wont benefit from using this the pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code in particular already existed in the v-next branch, I just moved it from internal/cli/main.ts into here. Definitely something to think about moving forward though.


const config = imported.default;

if (typeof config !== "object" || config === null) {
Copy link
Member

Choose a reason for hiding this comment

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

@alcuadrado This would allow Array configs to be passed, is that intended? By the way, this is the second time I've seen an isObject check (which is also missing the Array check). I believe we should add this as a util.

Copy link
Member

Choose a reason for hiding this comment

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

@zoeyTM you can ignore this, I'll put a separate PR adding isObject and replacing the occurrences with the helper.

@zoeyTM zoeyTM merged commit 0530527 into v-next May 28, 2024
44 checks passed
@zoeyTM zoeyTM deleted the config-loading branch May 28, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants