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

chore: update some nonbreaking major packages #26709

Closed
wants to merge 44 commits into from

Conversation

hoobdeebla
Copy link
Contributor

@hoobdeebla hoobdeebla commented Aug 30, 2020

Updates the following dependencies to newer (sometimes major) versions but does not break any code.

Package Type Change Breaking Changes
better-opn deps 1.0.0 -> ^2.0.0 none
documentation both ^12.3.0 -> ^13.0.2 require Node 10
eslint-loader deps ^2.2.1 -> ^4.0.2 require Node 10.13
eslint-plugin-flowtype deps ^3.13.0 -> ^4.7.0 require ESLint 6
eslint-plugin-graphql deps ^3.1.1 -> ^4.0.0 require Node 10
eslint-plugin-react-hooks deps ^1.7.0 -> ^4.1.0 catch additional Rules of Hooks violations
fast-levenshtein deps ^2.0.6 -> ^3.0.0 none
find-yarn-workspace-root deps ^1.2.1 -> ^2.0.0 require Node 8.6
globby deps ^10.0.2 -> ^11.0.1 require Node 10
inquirer deps ^6.5.2 -> ^7.3.3 require Node 8
markdown-magic devDeps ^0.2.1 -> ^1.0.0 none
md5-file deps ^3.2.3 -> ^5.0.0 require Node 10.13, return a Promise instead of providing a callback
micromatch deps ^3.1.10 -> ^4.0.2 require Node 8.6
mini-css-extract-plugin deps ^0.8.1 -> ^0.11.2 none
mitt deps ^1.2.0 -> ^2.1.0 none since we don't pass an object to mitt
node-fetch deps ^1.7.3 -> ^2.6.1 none that affect usage in gatsby-source-graphql
npm-package-arg deps ^6.1.1 -> ^8.0.1 require Node 10
null-loader deps ^3.0.0 -> ^4.0.0 require Node 10.13
p-queue deps ^2.4.2 -> ^6.6.1 change CommonJS import syntax
parse-numeric-range deps ^0.0.2 -> ^1.2.0 call module via default import instead of .parse() method
pify deps ^3.0.0 -> ^5.0.0 require Node 10
plop devDeps ^1.9.1 -> ^2.7.4 require Node 8.9.4
preval.macro devDeps ^3.0.0 -> ^5.0.0 require Node 10
puppeteer deps ^3.3.0 -> ^5.2.1 none that affect its usage in gatsby-plugin-preload-fonts
rewire devDeps ^4.0.1 -> ^5.0.0 require Node 8
string-similarity deps ^1.2.2 -> ^4.0.2 none
strip-ansi deps ^5.2.0 -> ^6.0.0 require Node 8
tmp-promise devDeps ^2.1.0 -> ^3.0.2 none
toml deps ^2.3.6 -> ^3.0.0 reverts behavior from 2.3.6, identical to 2.3.5
v8-compile-cache deps ^1.1.2 -> ^2.1.1 require Node 6
webpack-merge deps ^4.2.2 -> ^5.1.4 require Node 10

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Aug 30, 2020
@hoobdeebla hoobdeebla changed the title chore(gatsby): update nonbreaking major packages chore: update some nonbreaking major packages Aug 30, 2020
@hoobdeebla hoobdeebla marked this pull request as ready for review August 31, 2020 03:57
@sidharthachatterjee
Copy link
Contributor

@hoobdeebla Thank you so much for doing this! Can you fix conflicts? I'll get this in after that 🙌

@sidharthachatterjee sidharthachatterjee removed the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Aug 31, 2020
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Thank you, some small questions about package changes.

@@ -10,7 +10,7 @@ import { createUrqlClient } from "../../urql-client"
import { useMutation, useSubscription } from "urql"

import lodash from "lodash"
import fetch from "isomorphic-fetch"
import fetch from "cross-fetch"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why switch to cross-fetch?

Suggested change
import fetch from "cross-fetch"
import fetch from "cross-fetch"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the cross-fetch README:

Why not isomorphic-fetch?
My preferred library used to be isomorphic-fetch but it has this bug that prevents it from running in a react native environment. It seems unlikely to be fixed since there haven't been any new commits to it since 2016. That means dependencies are outdated as well.

Copy link

Choose a reason for hiding this comment

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

Additionally, isomorphic-fetch uses node-fetch@1, which is vulnerable to CVE-2020-15168 (disclosed yesterday).

Copy link

@G-Rath G-Rath Sep 11, 2020

Choose a reason for hiding this comment

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

In saying that, cross-fetch uses exact version constraints so is also currently affected by the same advisory

They're released a new version that updates node-fetch 🎉

Copy link
Contributor

Choose a reason for hiding this comment

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

Pulled out this security fix into its own PR here: #26876

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @karlhorky! Beat me to it haha

@@ -2,7 +2,7 @@
"use strict";
const fs = require("fs");
const path = require("path");
const glob = require("glob");
const glob = require("globby");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why switch here but not on different places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gatsby-theme was using both glob and globby. Including two different glob libraries in one package seemed redundant to me, so I just chose one over the other. glob works fine everywhere else, so I didn't change it across the whole monorepo

@hoobdeebla
Copy link
Contributor Author

hoobdeebla commented Sep 2, 2020

The updates added since the last review are as follows:

Package Type Change Breaking Changes
agadoo devDeps ^1.1.0 -> ^2.0.0 none that affect usage in gatsby-design-tokens
ast-types deps ^0.13.3 -> ^0.14.2 none that affect usage in gatsby-transformer-react-docgen
babel-plugin-react-css-modules deps ^3.4.2 -> ^5.2.6 require Babel 7
axios deps ^0.19.2 -> ^0.20.0 none
cache-manager deps ^2.11.1 -> ^3.4.0 none
change-case deps change-case ^3.1.0 -> camel-case ^4.1.1 split methods into lerna-managed packages
coffee-loader deps ^0.9.0 -> ^1.0.0 require Node 10.13
commander deps ^2.20.3 -> ^6.1.0 require Node 6
csvtojson deps ^1.1 -> ^2.0.10 rewrite for Promise/async support
dataloader deps ^1.4.0 -> ^2.0.0 none that affect usage in gatsby-plugin-mdx
debug both ^3.2.6 -> ^4.1.1 require Node 6
deep-map-keys deps ^1.2.0 -> ^2.0.1 require Node 6
escape-string-regexp deps ^1.0.5 -> ^4.0.0 require Node 10
execa deps ^3.4.0 -> ^4.0.3 require Node 10
file-type deps ^12.4.2 -> ^15.0.1 remove .minimumBytes (replace with 4100), rewrite API as async
fs-extra both ^8.1.0 -> ^9.0.1 require Node 10, use native recursive fs.mkdir for ensureDir* where possible
got deps ^8.3.2 -> ^10.7.0 require Node 10
guess-webpack deps ~0.4.17 -> ^0.4.19 none
jest-environment-jsdom-fourteen devDeps ^0.1.0 -> ^1.0.1 none
jest-worker deps ^24.9.0 -> ^26.3.0 require Node 10.13
js-combinatorics devDeps ^0.5.5 -> ^1.4.5 baseN() -> new BaseN(), move CommonJS export from . to ./commonjs/combinatorics
jscodeshift devDeps ^0.7.0 -> ^0.11.0 require Node 10
json2csv devDeps ^3.11 -> ^5.0.1 json2csv({ data, fields }) -> json2csv(data, { fields })
raw-loader deps ^0.5.1 -> ^4.0.1 require Node 10.13
subfont deps ^4.2.2 -> ^5.2.2 require Node 10
url-loader deps ^1.1.2 -> ^4.1.0 require Node 10.13
webpack-stats-plugin deps ^0.3.1 -> ^0.3.2 none
xlsx deps ^0.15.6 -> ^0.16.7 none that affect usage in gatsby-transformer-excel
yargs devDeps ^10.1.2 -> ^16.0.3 require Node 10
zipkin-javascript-opentracing devDeps ^2.1.0 -> ^3.0.0 none that affect usage in gatsby
zipkin-transport-http devDeps ^0.19.2 -> ^0.22.0 none
zipkin devDeps ^0.19.2 -> ^0.22.0 none

@hoobdeebla hoobdeebla marked this pull request as draft September 8, 2020 19:07
@wardpeet
Copy link
Contributor

@hoobdeebla Thank you for putting in the effort. As we've seen in the past, updating packages comes with a risk. Even if there are no breaking changes, there might be subtle ones (#26259).

We're happy to accept partial updates in smaller PRs. Perhaps update them by plugin 🤷. I've also updated our renovate config to make it easier for us to merge renovatebot PRs
#26892

@wardpeet wardpeet closed this Sep 15, 2020
@karlhorky
Copy link
Contributor

Thanks @wardpeet for focusing on stability!! I've been thinking and talking about this a lot lately: https://twitter.com/karlhorky/status/1304107463764045824

Smaller PRs would be great, maybe with adding some tests with each one.

@wardpeet
Copy link
Contributor

I'm reworking our circle-ci tests so we can add more e2e-tests to cover more ground.

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.

5 participants