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

Release - 1.2.7 #3470

Merged
merged 8 commits into from
Apr 24, 2020
Merged

Release - 1.2.7 #3470

merged 8 commits into from
Apr 24, 2020

Conversation

ryanio
Copy link
Collaborator

@ryanio ryanio commented Apr 15, 2020

Description

This PR introduces web3.js version 1.2.7 beginning with 1.2.7-rc.0.

Please see the release notes for more.

Changelog

Added

Changed

Fixed

Compare View

v1.2.6 -> v1.2.7

Type of change

  • Release

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run dtslint with success and extended the tests and types if necessary.
  • I ran npm run test:unit with success.
  • I have executed npm run test:cov and my test cases do cover all lines and branches of the added code.
  • I ran npm run build-all and tested the resulting file/'s from dist folder in a browser.
  • I have updated the CHANGELOG.md file in the root folder.
  • I have tested my code on the live network.

@ryanio
Copy link
Collaborator Author

ryanio commented Apr 15, 2020

Hey everyone, @cgewecke and I are happy to announce release version 1.2.7.

We plan to leave this PR open for one week (until next Tuesday, April 21) for community feedback and reviewer approvals. If there are any large changes requested, they will be published and released as incremented rc releases.

@michaelsbradleyjr @alcuadrado @gnidan hey 👋 please let us know if you could take a 👀 and give us your 👍 for this release. Thank you!

@ryanio ryanio added 1.x 1.0 related issues Release labels Apr 15, 2020
@cgewecke
Copy link
Collaborator

cgewecke commented Apr 15, 2020

As part of the RC process, am doing some test installations in other projects and running their tests to see if everything.

  • installs properly
  • builds
  • tests pass

This list will be actively edited:

Project Desc Status
oz-test-helpers Test utils. Subset of their suite is web3 contracts
oceanprotocol/squid-js TS API ~95 tests + tsc + webpack build Only used by sub-deps
gnosis/dex-react TS React app ~200 tests + webpack build + jest ✅ (Test/Build)

@nivida
Copy link
Contributor

nivida commented Apr 15, 2020

Is there a compare view existing to see the diff of 1.2.6 and 1.2.7 as in all other releases? also, the changelog we always had and which notified the issue/PR creators is missing in the description? can you reference the release guidelines as well? and is the RC version already released because I can't see the in-progress label? (I know some annoying questions) :-)

@ryanio ryanio added the In Progress Currently being worked on label Apr 15, 2020
@@ -119,6 +119,7 @@ var ugliyOptions = {
}
};

// Apply lerna.json version to root package.json
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these comments are non-related to this PR.. see point 4.1 of the contribution guidelines.

@ryanio
Copy link
Collaborator Author

ryanio commented Apr 15, 2020

Is there a compare view existing to see the diff of 1.2.6 and 1.2.7 as in all other releases?

Yes thanks, just added!

also, the changelog we always had and which notified the issue/PR creators is missing in the description? can you reference the release guidelines as well?

Will add now.

and is the RC version already released because I can't see the in-progress label? (I know some annoying questions) :-)

Just added, thanks ;)

package.json Show resolved Hide resolved
@@ -20,10 +20,11 @@
}
],
"scripts": {
"version": "npm run build-all",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't those modified or additional commands not be a separate PR to be aligned with the contribution guidelines from @cgewecke?

Copy link
Contributor

Choose a reason for hiding this comment

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

NOT a good idea to have a script named the same as a discrete step in the NPM release-lifecycle unless you fully intend to follow the documented behavior (not sure lerna can/will/do/acknowledge that re: the root package.json in a lerna publish scenario)...

Run AFTER bumping the package version, but BEFORE commit.

See: https://docs.npmjs.com/misc/scripts

Maybe that's the case, but NOTE-ing out of caution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes thanks that was exactly our intention, it is to build the minified file after bumping package versions but before committing the working state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... not entirely sure, but sounds like maybe "files" in package.json (in child packages) could be added / edited; and that .gitignore (root or children) could be adjusted to ignore *.min.js. Main point: if you're specifying a "version" script, something's probably not right.

It's been awhile since I looked closely at this monorepo, but if you have .npmignore in root or child packages, nuke with prejudice and prefer "files" in all child packages' package.json — it will save you a world of trouble.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, consider "prepublishOnly'" in package.json scripts.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... not entirely sure, but sounds like maybe "files" in package.json (in child packages) could be added / edited; and that .gitignore (root or children) could be adjusted to ignore *.min.js. Main point: if you're specifying a "version" script, something's probably not right.

I agree.. I've added the filesproperty to the package.json files in 2.x...

and that .gitignore (root or children) could be adjusted to ignore *.min.js.

Sadly no.. I've noticed the min file has to be on GitHub because many do use it for their tutorials and courses or of whatever reason :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

@ryanio

NOT a good idea to have a script named the same as a discrete step

I would add this command/alias to the required places without defining a version command because this is for some/many devs a "hidden logic" :D

Copy link
Collaborator

@cgewecke cgewecke Apr 15, 2020

Choose a reason for hiding this comment

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

There's more about this in the lerna documentation for version command lifecycle scripts.

@@ -132,7 +133,7 @@
"karma-firefox-launcher": "^1.2.0",
"karma-mocha": "^1.3.0",
"karma-spec-reporter": "0.0.32",
"lerna": "^3.18.3",
"lerna": "^3.20.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly was the reason to bump two minor versions of lerna? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

The lerna project is quite good about adhering to semver, so this should only pick-up bug fixes and new featuresl

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I agree and know.. just thought it is out of the scope of this PR idk (4.1 contribution rules) :-)

Copy link
Collaborator

Choose a reason for hiding this comment

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

For additional background: the existence of several moderate/high vulnerability audit warnings originating in Lerna was flagged by a longtime contributor in this 2744 comment, yesterday.

@nivida
Copy link
Contributor

nivida commented Apr 15, 2020

v1.2.6...v1.2.7-rc.0#diff-40a5d984375cfacc5cc552a01648ee51R20

Could we not add this disclaimer to the README.md and documentation and remove the in-line comment? This information is probably important for some old projects which are integrating web3.js idk.. (only the duplicated comments everywhere should have brought that up) :-)

I mean this says 3.7.. do we now have 3.0 or 3.7 as min. version? :P

v1.2.6...v1.2.7-rc.0#diff-8c363e4016c0523ddfd8355b2b3dcf48L26

Did this probably also improve the Promise chaining functionality? We do have 1-2 issues about it here on GitHub.

v1.2.6...v1.2.7-rc.0#diff-b9a9f37d5a3a90f3953a269ddca60a65R62

Typo it should be: // Subscription might not exist

v1.2.6...v1.2.7-rc.0#diff-a383cea7a2a9d92a58631e2d44a94ffeR201

@cgewecke is there a way to document the timing issue we noticed? 🤔

v1.2.6...v1.2.7-rc.0#diff-5ca1164e985980d3e045ad9298bbec26R9

Is this required or do I/We have a badly defined type? idk..

v1.2.6...v1.2.7-rc.0#diff-bd11c69500f2885e9e6ebfaf8fd2da13R26

I think this comment is I think from my side.. 😅 but we should probably create an issue out of it to track it idk... and this is a lie the generic Contract was/is always implemented.

class MyContract extends eth.Contract {}

v1.2.6...v1.2.7-rc.0#diff-7fb0ce5099d9515831133d423fef6c75R30

Can we add the explanation of it to the changelog or so? otherwise, those addresses do look like cheeked in...

revert msg sendSignedTransaction

As I informed Chris already is the additional network request not required.. should we inform the user about it? or do you think as I'm doing that this isn't required for the time being?

Thanks for all of your great work on this release! I'm really happy to see my started improvements are getting done and improved! ❤️

@cgewecke
Copy link
Collaborator

@nivida In your comment with detailed review suggestions, the links aren't resolving correctly. When I click on the diff hashes it shows "nothing to compare" page. Are you also seeing this on your side?

The suggested change to sendSignedTransaction is being tracked in #3457. It's an efficiency improvement that won't change correctness. Unsure what we should "inform the user about" here - could you clarify?

Unfortunately your suggestion about that came right as we agreed to freeze public-facing changes to prepare for a release so it will need to go in 1.2.8.

@ryanio
Copy link
Collaborator Author

ryanio commented Apr 17, 2020

I mean this says 3.7.. do we now have 3.0 or 3.7 as min. version? :P

dtslint was only checking TypeScript 3.9 prior to #3464. There were some type errors on earlier versions, which is why you see those minimum comments, it's actually to programmatically let dtslint know. I've been meaning to go back and see if I can resolve them since it's just a few of them out of all the packages, but haven't gotten a chance yet.

edit: resolved in #3479

@ryanio
Copy link
Collaborator Author

ryanio commented Apr 22, 2020

Just an update since today was our planned day for release.

@cgewecke and I still have a few review comments we are addressing from @nivida before considering ready so please give us some additional time to finish the aforementioned threads.

@cgewecke
Copy link
Collaborator

cgewecke commented Apr 24, 2020

@ryanio ryanio removed the In Progress Currently being worked on label Apr 24, 2020
Copy link
Collaborator

@cgewecke cgewecke left a comment

Choose a reason for hiding this comment

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

Yes, great work @ryanio! 🎉

@ryanio ryanio merged commit 2e4bbf4 into 1.x Apr 24, 2020
@holgerd77 holgerd77 deleted the release/1.2.7 branch April 24, 2020 20:42
@holgerd77
Copy link
Collaborator

Congrats! 🐡🐠🐟

@paub444
Copy link

paub444 commented Jul 25, 2021

I'm facing an issue when I do require('web3') inside a worker. It works for the first call, but nodejs crashes on the second call to the program.
Strangely, this issue happens when I use require('web3') in the main thread. I cannot avoid this because I need web3 in the main thread as well.

Is there an issue with updation of require cache by any chance?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants