-
Notifications
You must be signed in to change notification settings - Fork 777
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
New Releases (EVM/VM Event Emitter Fix) #2377
Conversation
…ncies (Util v8.0.2)
…ncies (EVM v1.2.0)
…ncies (VM v6.2.0)
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
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.
One question about version numbers but LGTM!
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "@ethereumjs/evm", | |||
"version": "1.1.0", | |||
"version": "1.2.0", |
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 guess I just noticed this but any reason we're using minor version updates for evm
and point releases for the other packages?
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.
Yes. I already used a minor for the first release due to the significance of the event emitter update, for the VM as well, so to indicate that this would be a release with substantial changes (which was obviously still not enough 😋).
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.
Fair. Lets merge 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.
Thanks for fixing this so quickly!
One note: for npm packages, package version numbers don't communicate significance (except version 1.0.0
) or provide any indication of how substantial the changes are. They indicate if the release contains fixes/refactors (0.0.x), new features (0.x.0), or breaking changes (x.0.0). Of course, if ethereumjs isn't adhering to Semver this isn't true, but we've been operating under the impression that it does.
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.
Hi there, yeah, Andrew did a really great job here! 🙂 👍
I've just published all the versions, hope everything works now, otherwise let us know (respectively: some general feedback would likely be good in this case).
Yes, I know that (semver). We DO adhere to Semver. For the bugfix vs minor versions I perceive these lines as a bit blurry and not as important as for minor vs breaking. Feature additions are often so insignifcant for the user - if a new function is mainly for internal usage e.g., or if their is a slight API parameter change or so. If one would always call this out a minor version, one would very quickly land at v6.45. 😋
So I switched a bit (more) to a distinction to "what feels right to me" in these cases and I reserve minor versions more for the a bit more substantal feature additions.
Richard (from Ethers) is doing the opposite and is putting everything in bugfix releases. 😆
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2022 EthereumJS Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
@acolytec3 @holgerd77 - thanks a lot! Only annoying bit is that I still need to add the following line to my "resolutions": {
"@ethereumjs/util": "8.0.2"
} But that is a minor hiccup, I really appreciate all the work on the event emitter! 🙌 |
Yeah, sorry, this will get better on the next round of full-scale releases, I was honestly too lazy to do the full round of 13 releases and chose to limit to the core 3 libraries. Super great to hear that things are working on the event emitter side!!! 🙏 We didn't have such a side-kicking bug/malfunction for quite some time, super-glad that this seemed to have worked out as expected/hoped, thanks again @acolytec3 for working through this. |
These releases are fixes for the previous EVM v1.1.0 and VM v6.1.0 releases from #2361 and #2371 where the async event emitter package had been updated which turned out to be breaking in some parts of the functionality, see comments made here #2303 (comment).
The new releases switch back to a modernized version of the initially used
async-eventemitter
package, see #2376.Open for review. 🙂
//cc @davidmurdoch @thevaizman