-
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
Network Manager M1 remaining tasks #5762
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
v-next/hardhat/test/internal/builtin-plugins/network-manager/hook-handlers/config.ts
Outdated
Show resolved
Hide resolved
validationErrors.length > 0, | ||
"validation errors should be present", | ||
); | ||
// TODO: the error message should be "Expected a URL or a Configuration Variable" |
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.
The interesting thing is that this validation uses conditionalUnionType
behind the scenes, so it might be bugged as well.
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 think this is working as intended concerning the conditionalUnionType
. Its main idea is to return errors that are as specific as possible.
unionType
does the contrary. It always returns a less specific error message.
Maybe for this particular case we should use unionType
.
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.
the problem is that unionType
expects the first argument to be an array of zod types, and we're using isObject
in sensitiveUrlType
. Maybe we could replace it with z.object()
or z.record()
, but we'd need to make it generic.
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've updated sensitiveUrlType
and sensitiveStringType
to use unionType
(and renamed them to schemas). However, there's a bug in Zod that prevents the union from using the provided errorMap
when the string schema is z.string().url()
(and likely other validations). See:
hardhat/v-next/hardhat-zod-utils/src/index.ts
Lines 156 to 160 in ae00903
* TODO: The custom error message in the unionType function doesn't work | |
* correctly when using string().url() for validation, see: | |
* https://github.com/colinhacks/zod/issues/2940 | |
* As a workaround, we provide the error message directly in the url() call. | |
* We should remove this when the issue is fixed. |
bb29501
to
9b46702
Compare
v-next/hardhat/src/internal/builtin-plugins/network-manager/type-validation.ts
Outdated
Show resolved
Hide resolved
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 left a few comments, but LGTM
c5f2a4a
to
3541c81
Compare
3541c81
to
ae00903
Compare
This PR addresses most of the tasks outlined in this document, except for the "Types" section, which will be addressed in a separate PR. I suggest reviewing one commit at a time for an easier review process.
Summary:
extendUserConfig
,validateUserConfig
,resolveUserConfig
).HTTPProvider
:jsonRpcWrapper
andclose
functions.NetworkManager
:connect
function and all network hooks (newConnection
,closeConnection
,onRequest
).NetworkConnection
: class initialization andclose
function (throughNetworkManager
)