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

deps: upgrade deps to get rid of Buffer constructor #6208

Merged
merged 1 commit into from
Aug 10, 2018

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Aug 2, 2018

Summary

Node 10 is about to become LTS and Yarn users are still receiving warnings from the use of the deprecated Buffer constructor. This PR updates the dependencies triggering those warnings.

Motivated by #5934 (comment) (thank you @jacobq).

We've come from a long way to get rid of those warnings in yarn. Here the list of the packages which have been upgraded related to the issue #5477:

The list of dependencies that are fixed by this PR:

  • asn1 (0.2.3 -> 0.2.4)
  • bl (BufferList) (1.2.1 -> 1.2.2)
  • buffer-alloc (1.1.0 -> 1.2.0)
  • duplexify (3.5.0 -> 3.6.0)
  • ecc-jsbn (0.1.1 -> 0.1.2)
  • peek-stream (1.1.2 -> 1.1.3)

When running yarn build-bundle then searching for all the occurrences of new Buffer(, there are now 14 search results:

  • 2 are comments
  • 8 are compatibility fallback
  • 1 is from buffer-fill, but the code is not used by any of the yarn commands
  • 2 are from chardet (might be used when running upgrade-interactive)
  • 1 is from http-signature

To sum up, deprecation warning in yarn is just two PR away of being just a bad memory:

Test plan

CI

@aduh95
Copy link
Contributor Author

aduh95 commented Aug 2, 2018

The PR on asn1 has been merged, I will rebase as soon as they release a new version with the fix.

@buildsize
Copy link

buildsize bot commented Aug 2, 2018

File name Previous Size New Size Change
yarn-[version].noarch.rpm 920.27 KB 1.01 MB 113.15 KB (12%)
yarn-[version].js 4.02 MB 3.88 MB -143.82 KB (3%)
yarn-legacy-[version].js 4.18 MB 4.05 MB -141.91 KB (3%)
yarn-v[version].tar.gz 925.9 KB 1.01 MB 113.11 KB (12%)
yarn_[version]all.deb 675.37 KB 745.66 KB 70.29 KB (10%)

@arcanis
Copy link
Member

arcanis commented Aug 2, 2018

Great work, thanks! Can you check whether the tests run? It's curious they pass on Node 4 but not the others 😕

@aduh95 aduh95 force-pushed the no_more_buffer_constructor branch from 66ce7e1 to 9a056b3 Compare August 2, 2018 20:37
@aduh95
Copy link
Contributor Author

aduh95 commented Aug 2, 2018

I am not sure of what happended there, I am running Node 8 and I have few tests failing on my machine due to timeout: I blame my connection, bet let's see if the CI will say this time.

Now asn1 has released a new version, I have rebased my branch and update the dependencies. Somehow, there is now a new package in the dependency tree which is using the deprecated Buffer constructor... I have updated the PR description with the new data.

yarn why v1.10.0-0
[1/4] Why do we have the module "chardet"...?
[2/4] Initialising dependency graph...
[3/4] Finding dependency...
[4/4] Calculating file sizes...
=> Found "chardet@0.4.2"
info Reasons this module exists
   - "external-editor" depends on it
   - Hoisted from "external-editor#chardet"
info Disk size without dependencies: "108KB"
info Disk size with unique dependencies: "108KB"
info Disk size with transitive dependencies: "108KB"
info Number of shared dependencies: 0
$ bin/yarn why external-editor
=> Found "external-editor@2.2.0"
info Has been hoisted to "external-editor"
info Reasons this module exists
   - Hoisted from "inquirer#external-editor"
   - Hoisted from "commitizen#opencollective#inquirer#external-editor"
info Disk size without dependencies: "72KB"
info Disk size with unique dependencies: "628KB"
info Disk size with transitive dependencies: "704KB"
info Number of shared dependencies: 5
=> Found "commitizen#external-editor@1.1.1"
info Reasons this module exists
   - "commitizen#inquirer" depends on it
   - Hoisted from "commitizen#inquirer#external-editor"

@aduh95
Copy link
Contributor Author

aduh95 commented Aug 2, 2018

So now the linter crashes, I guess some unrelated dependencies have upgraded to version containing breaking changes.

I am rolling back the version bump of the related dependencies and freezing the package to working version. Another PR should look into that.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@aduh95 aduh95 force-pushed the no_more_buffer_constructor branch from 4453f28 to a396308 Compare August 3, 2018 21:49
@arcanis
Copy link
Member

arcanis commented Aug 6, 2018

@imsnif Can you give a look at this PR? I'm not entirely sure I understand why the integrity hashes are changing like crazy by switching from sha-1 to sha-512 and vice versa.

@arcanis arcanis requested a review from imsnif August 6, 2018 18:04
@imsnif
Copy link
Member

imsnif commented Aug 6, 2018

Sure - I'll take a look sometime in the next two days.

Copy link
Member

@imsnif imsnif left a comment

Choose a reason for hiding this comment

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

This LGTM. I've sampled some integrity checksums and they seem to be behaving properly.
The checksums should change when a verion changes. Whether they are sha1 or sha512 depends on what's in the registry, which in turn depends on how the package was published. There are (for example) some packages here that have been "downgraded" from sha512 to sha1 (eg. aria-query: https://github.com/yarnpkg/yarn/pull/6208/files#diff-8ee2343978836a779dc9f8d6b794c3b2R270). I checked the registry and it checks out (the newer version of the package: 3.0.0 has a sha1 hash and the older one has a sha512 one - my guess is that this is because the newer one was published with a previous version of yarn that could not yet publish packages with a sha512 integrity hash).

While admittedly I did not check all packages (yarn should do that upon install :) ) - a sampling checked out. This is normal behaviour.

@arcanis
Copy link
Member

arcanis commented Aug 10, 2018

Interesting 😮 Good for me then! 👍

@arcanis arcanis merged commit dcb959d into yarnpkg:master Aug 10, 2018
@aduh95 aduh95 deleted the no_more_buffer_constructor branch August 10, 2018 13:04
@arcanis arcanis mentioned this pull request Aug 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants