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

New Releases (EVM/VM Event Emitter Fix) #2377

Merged
merged 4 commits into from
Oct 21, 2022

Conversation

holgerd77
Copy link
Member

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

@codecov
Copy link

codecov bot commented Oct 21, 2022

Codecov Report

Merging #2377 (f478c57) into master (b5a2f4b) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 91.37% <ø> (ø)
blockchain 90.21% <ø> (ø)
client 87.00% <ø> (ø)
common 98.13% <ø> (ø)
devp2p 91.76% <ø> (+0.21%) ⬆️
ethash ∅ <ø> (∅)
evm 79.20% <ø> (ø)
rlp ∅ <ø> (∅)
statemanager 89.46% <ø> (ø)
trie 90.02% <ø> (-0.35%) ⬇️
tx 97.80% <ø> (ø)
util 83.89% <ø> (ø)
vm 85.30% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@acolytec3 acolytec3 left a 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",
Copy link
Contributor

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?

Copy link
Member Author

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 😋).

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair. Lets merge it!

Copy link
Contributor

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.

Copy link
Member Author

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. 😆

@holgerd77 holgerd77 merged commit f886251 into master Oct 21, 2022
@gitpoap-bot
Copy link

gitpoap-bot bot commented Oct 21, 2022

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2022 EthereumJS Contributor:

GitPOAP: 2022 EthereumJS Contributor GitPOAP Badge

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

@holgerd77 holgerd77 deleted the new-releases-vm-event-emitter-fix branch October 21, 2022 16:02
@holgerd77
Copy link
Member Author

Just published the following releases on npm:

@thevaizman
Copy link

@acolytec3 @holgerd77 - thanks a lot!
I can confirm that everything seems to function well with our app too (evm.codes).

Only annoying bit is that I still need to add the following line to my package.json to prevent the blockchain package from throwing errors due to it picking up the 8.0.0 by default (even though it has ^8.0.0 as an inner dependency):

"resolutions": {
    "@ethereumjs/util": "8.0.2"
  }

But that is a minor hiccup, I really appreciate all the work on the event emitter! 🙌

@holgerd77
Copy link
Member Author

@acolytec3 @holgerd77 - thanks a lot! I can confirm that everything seems to function well with our app too (evm.codes).

Only annoying bit is that I still need to add the following line to my package.json to prevent the blockchain package from throwing errors due to it picking up the 8.0.0 by default (even though it has ^8.0.0 as an inner dependency):

"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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants