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.5 #3315

Merged
merged 12 commits into from
Jan 27, 2020
Merged

Release - 1.2.5 #3315

merged 12 commits into from
Jan 27, 2020

Conversation

nivida
Copy link
Contributor

@nivida nivida commented Jan 16, 2020

Description

We will first release version 1.2.5 as 1.2.5-rc.0 and will follow the release guidelines as described here.

Added

Changed

Fixed

Compare View

v1.2.4 -> v1.2.5

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 test with success and extended the tests if necessary.
  • I ran npm run build and tested the resulting file from dist folder in a browser.
  • I have tested my code on the live network.

BesrourMS and others added 3 commits December 14, 2019 22:29
Co-Authored-By: Samuel Furter <nivida@users.noreply.github.com>
…w version will be set for the version property in the minified file
@nivida nivida added Release 1.x 1.0 related issues Review Needed Maintainer(s) need to review labels Jan 16, 2020
@coveralls
Copy link

coveralls commented Jan 16, 2020

Coverage Status

Coverage remained the same at 85.36% when pulling e919f81 on release/1.2.5 into 513116f on 1.x.

@holgerd77
Copy link
Collaborator

I think it would eventually make sense to complement the common release guidelines with a simple release TODO list template which can then be copied over and worked down along a new release - seems like many of these steps are repeatable - so a list like:

  • Update version attribute in package.json
  • Update version attribute in packages/web3/package.json
  • Update minified version of the library at dist/web3.min.js
  • Update package-lock.json file
  • Check README.md for necessary changes
  • ...

We could actually use something like this for EthereumJS as well, let's really see if we can take parts / most of the rules you worked out here over once you guys collected some more experience with it.

BTW: what is your policy on documentation updates (eventually also something for the list above)? Do you update the docs also on release or along with PRs (and so to have them for some time in parts on a non-release matching state)? We recently had some debate on this on the EthereumJS side.

@nivida
Copy link
Contributor Author

nivida commented Jan 16, 2020

@holgerd77 Great idea! I do have such a list locally on my PC. I will update the release guidelines with such a list after the RC release got published.

BTW: what is your policy on documentation updates (eventually also something for the list above)? Do you update the docs also on release or along with PRs (and so to have them for some time in parts on a non-release matching state)? We recently had some debate on this on the EthereumJS side.

Until now did I update the documentation after a stable release got published (see versions in our readthedocs documentation). I thought to keep the documentation as it is until the production version got released but I will kick off a discussion about with @cgewecke. Thanks!

Btw.: Lerna does currently have a problem to detect the changes correctly.

@holgerd77
Copy link
Collaborator

👍

Docs: 1) Ah no, I think the variant you do as common practice (keep until release) makes most sense, we actually also realized that our up till recently executed practice of update docs along with PRs confuses people. 😋 2) So doc update practice is likely also a case for the TODO list (eventually to split this up into a "Before merge" and "After merge" list or similar, whatever makes sense to structure the bigger 2-3 overlaying steps).

@nivida
Copy link
Contributor Author

nivida commented Jan 16, 2020

@michaelsbradleyjr @gnidan @alcuadrado @iurimatias @spalladino @cgewecke @holgerd77 @sohkai @fabioberger @caleteeter @joshstevens19

I've published version 1.2.5-rc.0. Feel free to give any feedback about this version here in this PR. We will release the production version of 1.2.5 after a week but only if no changes are requested from any contributor or consumer of the web3.js library.

@cgewecke
Copy link
Collaborator

cgewecke commented Jan 16, 2020

@nivida Just checked npm and it looks like the tagging of the rc release should be adjusted. The new rc is set to latest. Running npm view web3 shows:

dist-tags:
latest: 1.2.5-rc.0   next: 2.0.0-alpha.1  

I think this can be addressed using npm's dist-tags command. Worth double-checking but maybe this?

npm dist-tag add web3@1.2.5-rc.0 rc
npm dist-tag add web3@1.2.4 latest

It looks like all the packages (e.g. web-utils) would need to be adjusted this way...

@nivida
Copy link
Contributor Author

nivida commented Jan 17, 2020

@cgewecke Thanks Chris for commenting here. As already discussed over Gitter with you did I have a short chat with Holger about it and the outcome was to keep the rc version on the latest track for now because it’s aligned with semver which means dynamically defined versions as for example ^1.2.4 do get handled correctly and the rc version doesn't get installed. We will move the rc version on a separate track with a 1.2.5-rc.1 release if such a release is required or at least on the next coming rc version of the 1.2.6 release.

@holgerd77
Copy link
Collaborator

@cgewecke I have a bit a mixed feeling here but in the end we have little "damage" here respectively we just don't get the full benefits of the new release process in this first round. Worst direct outcome is that some direct users will have a release candidate installed, which would have been the directly released version if we would have kept going on with the old release process.

For the next release we can see that we get the tagging right as well. @nivida maybe you can already post for discussion here the anticipated correct CLI publication commands which would have gotten the tagging right? Then we are better prepared on next round.

@cgewecke
Copy link
Collaborator

@nivida @holgerd77 Ok sounds good to me! 👍

@nivida
Copy link
Contributor Author

nivida commented Jan 17, 2020

@holgerd77 Sounds good! I will extend the scripts defined in the root package.json with a release:rc script which does define the tags correctly for each package and we can discuss it in the related PR. 👌

@alcuadrado
Copy link

I think there was a misinterpretation of how semver handles prerelease versions. At least npm's implementation doesn't behave as described by @nivida. This section explains it it: https://www.npmjs.com/package/semver#prerelease-tags

This broke Buidler, as it validates its plugins' dependencies. Some plugins have a dependency on web3@^1.2.0, and this isn't satisfied by 1.2.5-rc.0, as you can see here:

pato@pmbp:asd% node
> const semver = require("semver")
undefined
> semver.satisfies("1.2.5-rc.0", "^1.2.0")
false

This may be breaking other systems that inspect dependencies versions, like web bundlers.

@nivida
Copy link
Contributor Author

nivida commented Jan 18, 2020

@alcuadrado

which means dynamically defined versions as for example ^1.2.4 do get handled correctly and the rc version doesn't get installed.

As described above and quoted here does the rc version no get installed if the version range is specified as ^1.2.0 and I think this is the correct behavior and does respect semver

@alcuadrado
Copy link

Just to keep everyone else informed, we discussed this more with @nivida through discord. The problem is when someone installs web3 directly, and another package that depends on it. For example npm i -D @nomiclabs/buidler @nomiclabs/buidler-web3 web3 is very common.

The problem is that npm i web3 means npm i web3@latest, which in turn means npm i web3@1.2.5-rc.0. Meanwhile, any package depending on web3 will probably have a dependency on web3@^1.2.x with x < 5, as 1.2.5 hasn't been published yet. When npm has to resolve that, it will choose the highest version that satisfies that requirement, and 1.2.5-rc.0 doesn't.

What are the implications of this? For systems that validate their dependencies, like Buidler, it will lead to a failure. For bundlers, it will probably lead to web3 being duplicated, as it's easy to have two versions installed.

@michaelsbradleyjr
Copy link
Contributor

michaelsbradleyjr commented Jan 18, 2020

@alcuadrado has a very good point.

While it's only a convention, it seems to me that the implicit @latest NPM dist-tag means, effectively, "latest stable release". By implicit I mean what @alcuadrado wrote above:

The problem is that npm i web3 means npm i web3@latest...

For that reason, I think it's best that only non-prerelease versions (stable versions) are dist-tagged as latest.

Likewise, prereleases should use another dist-tagging convention, e.g. rc, or next, or beta, etc. Note that dist-tags are orthogonal to semver prerelease identifiers, though they can be made to align reasonably well. Also note that a package version can have multiple dist-tags. And also note that there are some restrictions on dist-tags, i.e. they can't be valid semver.

Hypothetical convention for package dist-tags:

next — latest release candidate or latest stable release, regardless of major version
next-v1 — latest release candidate or latest stable release for major version 1

(the next two are superfluous if a project strictly adheres to semver; mega-bonus 🎁 if the project strictly adheres to Conventional Commits)

next-v1.2 — latest release candidate or latest stable release for minor version 1.2
next-v1.2.3 — latest release candidate or latest stable release for patch version 1.2.3

The reason for the "...or latest stable release" clauses above is owing to a concern about version drift in the dist-tags.

Imagine that next tags are not updated to point to the latest stable version and that some downstream project depends on web3, web3-foo, and web3-bar. The project maintainer wants to check for compatibility with the upcoming (@next) versions and in package.json specs web3@next, web3-foo@next, web3-bar@next. Seems reasonable, but suppose that web3 and web3-foo are in prerelease while web3-bar is still stable 🤔 💣 ...web3-bar@next would resolve to a prerelease predating its current stable version. 💥

By updating next tags to always point to either the most recent stable version (after graduating from prerelease) or the most recent prerelease since the last stable release, the problem described above is avoided.


This subject recently came up re: planning for a nightly-release GitHub Actions workflow for the Embark Framework. We've settled on using a -nightly.[N] semver prerelease identifier (and will no longer use other prerelease identifiers such as -alpha.[N], -beta.[N], etc.) and a nightly dist-tag. Our stable release script updates each stably-released package's nightly dist-tag to point to its latest stable release, for the reasons described above.

When a major version is pending, we'll use a series of alpha-v[M], beta-v[M], rc-v[M] dist-tags (pointing to successive [M].0.0-nightly.[N] versions (or latest stable version; see above)) to step through a series of alpha/beta/rc candidates prior to the major-level stable release.

We're also now using a --force-publish major-version strategy (pertains to lerna publish|version) such that, upon a major-level stable release, all packages are bumped to a new major version even if they weren't in prerelease. The reason for that, translated to web3, is that downstream projects can then confidently spec "web3[-*]: "^[M].0.0" in their package.json files ([M] could be 7, or whatever), knowing that all stable versions of web3[-*] packages will be at the same major version. Note that this can be relevant for the upstream project even when it uses Lerna's "fixed" versioning mode (despite what the README suggests), though it may only become an issue when the --conventional-commits option is used with lerna publish|version (which is the case for Embark).

@cgewecke
Copy link
Collaborator

cgewecke commented Jan 21, 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
gnosis/dex-react TS React app ~200 tests + webpack build + jest

(Some of these require quite a bit of manual dependency pruning to remove non-rc versions of Web3.)

@cgewecke cgewecke mentioned this pull request Jan 23, 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.

Would just like some clarity on the build status before approving - other than that - in testing everything has seemed correct to me.

@nivida
Copy link
Contributor Author

nivida commented Jan 24, 2020

Would just like some clarity on the build status before approving - other than that - in testing everything has seemed correct to me.

Great! Thanks for your analysis! I will check that as well:)
If anything is fine can we release the 1.2.5 production version on Monday 🎊

@cgewecke
Copy link
Collaborator

@nivida I'm very sorry - I misunderstood whether that version of the build should be included in the git release here. I do see it in node_modules/web3/dist when I install via npm - it looks fine.

@cgewecke cgewecke self-requested a review January 24, 2020 10:24
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.

Everything looks good to me!

@pcowgill pcowgill mentioned this pull request Jan 26, 2020
@nivida nivida removed the Review Needed Maintainer(s) need to review label Jan 27, 2020
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.

7 participants