Skip to content

Upgrade dependencies #676

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

Merged
merged 8 commits into from
Apr 12, 2018
Merged

Upgrade dependencies #676

merged 8 commits into from
Apr 12, 2018

Conversation

mmermerkaya
Copy link
Contributor

@mmermerkaya mmermerkaya commented Apr 12, 2018

Upgrades all dependencies to their latest versions based on the version range specified in the package.json file.

I mainly need the TS upgrade, but we might as well update all packages while we're at it.

FYI: JS/TS file changes are caused by Prettier upgrade.

@mmermerkaya mmermerkaya requested a review from jshcrowthe April 12, 2018 11:23
@mmermerkaya mmermerkaya changed the title Update dependencies Upgrade dependencies Apr 12, 2018
Copy link
Contributor

@jshcrowthe jshcrowthe left a comment

Choose a reason for hiding this comment

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

Running yarn upgrade will only update to the latest compatible versions within the ranges specified in the package.json.

If you want to actually update the versions, I believe you want to run yarn upgrade --latest. Typically in the past when I have done this, I do yarn upgrade-interactive --latest as that lets me be conscious of which upgrades I'm taking.

@mmermerkaya
Copy link
Contributor Author

Yes, I'm aware of that, but our version ranges are defined with ^, and that makes yarn upgrade update to the latest non-breaking version, which is what I need :)

@jshcrowthe
Copy link
Contributor

Fair enough. To the degree that we can, I'd like the package.json to remain up to date with the versions we are using (not just through the range). So can we also update the associated package.json?

@mmermerkaya
Copy link
Contributor Author

That's the job of yarn.lock though. It keeps track of the exact versions we're using.

If you feel strongly about this, we can remove ^ ranges from package.json, but I think it makes more sense to keep it, so we can upgrade to non-breaking versions using yarn upgrade, and fully upgrade using upgrade --latest. WDYT?

@jshcrowthe
Copy link
Contributor

Here's a scenario (that we've actually encountered) where not updating the package.json breaks things.

  • A component is built using a package (in this case closure-builder).
  • closure-builder releases a new range release (compatible with our version range) that fixes a bug
  • We upgrade the yarn.lock to consume this dependency

At this point in our local development two things are now true:

  • Locally built packages have updated to use the new dependencies
  • Locally the bug is fixed

However from the individual component point of view (i.e. packages/messaging), no code has actually changed so NPM publish is noop'ed and ultimately the bugfix doesn't make it out to users.

This is a problem unique to namespaces, if we were using yarn in a single package, updating the yarn.lock would result in a code diff which would signal that we need to do a new release, with workspaces this isn't the case.

Having the package.json file in each repo be the source of truth in this case fixes the issue. An update to the version indicated in the package.json, signals that we need a release.

@mmermerkaya
Copy link
Contributor Author

I see. In that case, what do you think about my suggestion of removing ranges from all package.jsons?

@jshcrowthe
Copy link
Contributor

I'm fine w/ that in principle, in practice we just need to make sure we are keeping up to date w/ version releases.

@mmermerkaya
Copy link
Contributor Author

Looks like we're not actually using ranges anyway, so I think it makes sense.

@jshcrowthe
Copy link
Contributor

SGTM, I know that for packages/webchannel-wrapper I did this, but I hadn't pinned the versions explicitly of any other devDependencies. If that's the case across the spectrum then let's update the package.jsons and move forward.

@mmermerkaya
Copy link
Contributor Author

mmermerkaya commented Apr 12, 2018

So it looks like yarn upgrade-interactive --latest doesn't change the package.json if the new version is within range, so if we want to use that instead of something like npm-check-updates, we'll have to remove ranges anyway...

I'll take care of it.

@jshcrowthe
Copy link
Contributor

I'll look for the update 😄

@mmermerkaya
Copy link
Contributor Author

mmermerkaya commented Apr 12, 2018

Done.

I updated all packages except Webpack (and loaders), as Webpack is already a major version behind.

Also couldn't update grpc in firestore as that update broke a test. I left a TODO.

Copy link
Contributor

@jshcrowthe jshcrowthe left a comment

Choose a reason for hiding this comment

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

Changes LGTM 👍

@mmermerkaya mmermerkaya merged commit aaee15d into master Apr 12, 2018
@mmermerkaya mmermerkaya deleted the mmermerkaya-dependency-upgrade branch April 12, 2018 20:46
@firebase firebase locked and limited conversation to collaborators Oct 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants