-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Release - 1.2.7 #3470
Conversation
Hey everyone, @cgewecke and I are happy to announce release version 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 @michaelsbradleyjr @alcuadrado @gnidan hey 👋 please let us know if you could take a 👀 and give us your 👍 for this release. Thank you! |
As part of the RC process, am doing some test installations in other projects and running their tests to see if everything.
This list will be actively edited:
|
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 |
@@ -119,6 +119,7 @@ var ugliyOptions = { | |||
} | |||
}; | |||
|
|||
// Apply lerna.json version to root package.json |
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 these comments are non-related to this PR.. see point 4.1 of the contribution guidelines.
Yes thanks, just added!
Will add now.
Just added, thanks ;) |
@@ -20,10 +20,11 @@ | |||
} | |||
], | |||
"scripts": { | |||
"version": "npm run build-all", |
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.
Shouldn't those modified or additional commands not be a separate PR to be aligned with the contribution guidelines from @cgewecke?
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.
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.
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 thanks that was exactly our intention, it is to build the minified file after bumping package versions but before committing the working state.
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.
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.
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.
Also, consider "prepublishOnly'"
in package.json
scripts.
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.
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 files
property 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 :-)
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.
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
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 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", |
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.
What exactly was the reason to bump two minor versions of lerna? 🤔
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 lerna project is quite good about adhering to semver, so this should only pick-up bug fixes and new featuresl
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.
Yep, I agree and know.. just thought it is out of the scope of this PR idk (4.1 contribution rules) :-)
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.
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.
Could we not add this disclaimer to the I mean this says 3.7.. do we now have 3.0 or 3.7 as min. version? :P
Did this probably also improve the
Typo it should be:
@cgewecke is there a way to document the timing issue we noticed? 🤔
Is this required or do I/We have a badly defined type? idk..
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 class MyContract extends eth.Contract {}
Can we add the explanation of it to the changelog or so? otherwise, those addresses do look like cheeked in...
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! ❤️ |
@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. |
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 |
|
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, great work @ryanio! 🎉
Congrats! 🐡🐠🐟 |
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. Is there an issue with updation of require cache by any chance? |
Description
This PR introduces
web3.js
version1.2.7
beginning with1.2.7-rc.0
.Please see the release notes for more.
Changelog
Added
Changed
docs/_build
folderFixed
fromBlock
doesnt return events from the past #3389)Compare View
v1.2.6 -> v1.2.7
Type of change
Checklist:
npm run dtslint
with success and extended the tests and types if necessary.npm run test:unit
with success.npm run test:cov
and my test cases do cover all lines and branches of the added code.npm run build-all
and tested the resulting file/'s fromdist
folder in a browser.CHANGELOG.md
file in the root folder.