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

Updates to latest dependencies, and transpilation targets #1658

Merged
merged 8 commits into from
Oct 3, 2018

Conversation

jeffposnick
Copy link
Contributor

@jeffposnick jeffposnick commented Sep 23, 2018

R: @philipwalton

Fixes #1654, fixes #1655, fixes #1656, fixes #1542, fixes #1574, to be merged into the next branch.

This has a whole host of updates to the various dependencies and devDependencies, bumping up most of them to the latest releases across all of our package.json.

Along with that, I've updated the transpilation targets to node 6 (which is required by some of the updated dependencies) and Chrome 56. (If it turns out that we need to stick with Chrome 51, we can change that back.)

There are a lot of whitespace changes in this PR, as the updated eslint config was not happy with some of our previous formatting. (Link to view with whitespace changes hidden.)

The webpack tests are currently failing, as I need to update a whole bunch of those constants in order to get them to pass. They need to be updated whenever any of the webpack-related dependencies change, and I'm going to hold off on remediating everything (or scripting an update) until I make sure that we don't have any other updates.

@jeffposnick jeffposnick added the Breaking Change Denotes a "major" semver change. label Sep 23, 2018
@jeffposnick jeffposnick added this to the v4 milestone Sep 23, 2018
"serve-index": "^1.9.1",
"service-worker-mock": "^1.7.2",
"service-worker-mock": "^1.9.3",
"shelving-mock-indexeddb": "github:philipwalton/shelving-mock-indexeddb#d2c5e52b6f9903026f5bac31cbe758cbebd98661",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philipwalton as far as I can tell, the change in your fork never made it to the upstream project, so we're still pinned to this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'll file an issue to upstream that.

"@babel/plugin-transform-runtime": "^7.1.0",
"@babel/preset-env": "^7.1.0",
"@google-cloud/storage": "^2.0.3",
"@octokit/rest": "^15.12.0",
"@std/esm": "^0.26.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried moving to the esm project (which replaced @std/esm), but there are a lot of breaking changes and I couldn't get our unit test runner happy. So we're still stuck on this older version until we can investigate further.

arcanis and others added 3 commits September 28, 2018 10:22
Copy link
Member

@philipwalton philipwalton left a comment

Choose a reason for hiding this comment

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

I wonder if it makes sense to split this into two different PRs. One that just handles the formatting changes, and one that handles all the other changes.

@@ -20,13 +20,14 @@ module.exports = {
rules: {
"jsdoc/check-types": 2,
"jsdoc/newline-after-description": 2,
'indent': ['error', 2],
Copy link
Member

Choose a reason for hiding this comment

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

What's this being added for?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, now that I'm looking at the rest of the changes, I see what you're doing. But technically this isn't consistent with Google style because in some cases Google style recommends indenting 4 spaces (e.g. line continuations and stuff like that).

The google eslint package v0.10.0 adds an indentation rule more inline with what's recommended by Google JS style. It's probably worth just upgrading to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed in person, there are going to be a lot of additional files that need whitespace adjustment if we don't include this.

We're already using "eslint-config-google": "^0.10.0" as part of this PR.

Let's do separate PR to remove this override and get all the whitespace consistent rather than including any more changes in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

@@ -32,7 +32,7 @@ module.exports = (packagePath) => {
// are only included in our Rollup bundles once, even if they're used
// in multiple source files.
// See https://github.com/rollup/rollup-plugin-babel#helpers
'transform-runtime',
'@babel/transform-runtime',
Copy link
Member

Choose a reason for hiding this comment

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

BTW, if we're getting rid of the regenerator runtime polyfill, we may not need this at all anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need it for the node libraries, since we're targeting node 6, and that doesn't support async/await.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@jeffposnick
Copy link
Contributor Author

I've merged in the 3.6.2 changes to this PR, just so we don't drift too far from master.

"serve-index": "^1.9.1",
"service-worker-mock": "^1.7.2",
"service-worker-mock": "^1.9.3",
"shelving-mock-indexeddb": "github:philipwalton/shelving-mock-indexeddb#d2c5e52b6f9903026f5bac31cbe758cbebd98661",
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'll file an issue to upstream that.

};
plugins.push(babel(babelConfig));

let minifyBuild = buildType === constants.BUILD_TYPES.prod;
if (minifyBuild) {
const uglifyOptions = {
const terserOptions = {
mangle: {
properties: {
Copy link
Member

Choose a reason for hiding this comment

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

I discovered recently that we probably need to enable the option to make property mangling consistent across files (when we're outputting multiple files). I can file an issue to do that in a separate PR.

@jeffposnick jeffposnick merged commit 7a65759 into next Oct 3, 2018
@jeffposnick jeffposnick deleted the package-json-updates branch October 3, 2018 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Denotes a "major" semver change.
4 participants