This repository has been archived by the owner on Feb 26, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Devise new experimental @truffle/from-hardhat package for compatibility translation #5420
Devise new experimental @truffle/from-hardhat package for compatibility translation #5420
Changes from all commits
e9da6d9
7168522
8f21732
b94a30b
19015c6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@cds-amal @fainashalts FYI I realized that we can benefit from Hardhat's own types by keeping this as a devDependency safely... if we just don't actually export anything with these types in the signature. This is the approach I'm taking here.
Just letting you know because this breaks the assumptions of our earlier conversation.
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.
how does this compare to making it an optional peerDepenency?
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.
Good question! Firstly, it's worth noting that
package.json
'speerDependenciesMeta
field (which is how you mark peerDeps as optional) was added in NPM v7, and we need to support v6 still. But this is a minor quibble.The main concern here, IMO, is the question "what does Truffle's Node.js-land need from the
"hardhat"
NPM package?"devDep
devDep
if we want to have a shared Node.js runtime between Truffle and Hardhat... as in, if we want to use Hardhat as a libraryI looked at using HH as a library, but, like Truffle, it's really not intended for use via straight
require()
(orimport
). I got it working in #5410... so this approach is viable albeit awkward. But think about what we're saying when Truffle doesrequire("hardhat")
... like, "here you go, Node! have another, different one of me!". Even without the risk of cross-clobbering each other's globals / other runtime modification shenanigans, the memory footprint and whatnot... oy!So my conclusion was basically... screw
peerDependencies
; HH is prescriptive: you use it vianpx hardhat
. Truffle can use HH vianpx hardhat
. Keep the playgrounds separate :)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.
Probably worth following up that there is a counter-argument that we could just use
peerDependencies
for the$(npm bin)
or whatever. But still... I think there's value in treating HH as an executable... like, you're not going to try to shove Docker into yourpackage.json
- you just shell out todocker
and error nicely if it's not there.Oh, and another follow-up argument might be: we could use
peerDependencies
to leverage npm/yarn's semver powers to avoid manual version compatibility checks ourselves at runtime... and yeah, we could, but I don't think it's worth the added surface area of complexity around managing this peerDependency... like, who gets the actual dependency? Truffle? Like, oof.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.
Ah that makes sense, and feel free to ignore my repeat question in the review below.
I was just thinking that it'd be very nice if there was some way to import just the types from the hardhat package without needing to add all of their transitive dependencies into our
node_modules
tree and ouryarn.lock
(even if it is just for dev purposes).If our usage of their types is minimal, perhaps it'd be worth extracting only the types that we need into a local type declaration? Though I could also easily argue against that approach due to the potential effort involved in doing that, along with the increased maintenance burden that will be required.
Sure would be nice if there was a code-gen utility for that sort of thing, though!
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.
Hm, I see your concern. Not sure copy+pasting the types is acceptable? Sounds like follow-on work? Or maybe we could get HH to publish types separately?
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.
Yeah, I think if we make changes to this approach, it's definitely something that's a future concern.
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.
Ah, I resolved this, but realized that neither of the two people who you pinged at the start have replied, so I've unresolved it.
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.
Otherwise please consider my part in this 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.
Worth noting about this approach: this only works because
hre.config
is serializable.I am so grateful and so jealous that they made this serializable :)
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.
Part of the struggle of forging our own path in the early days of the Ethereum ecosystem, I suppose. Also the node ecosystem and what is considered best practice has advanced just a touch since 2015!
Like, I think at the time that the original
truffle-config.js
was designed executable config was the hotness, as it was (and still is) super flexible. Now it's somewhat frowned upon for multi-modal projects like ours due this exact concern.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.
There's trade-offs... like, I also had the thought about how easy it is to read private keys from memory. Our approach clamps that down, since we only store mnemonics inside closures.