-
Notifications
You must be signed in to change notification settings - Fork 823
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
Conversation
"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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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.
* Fixes workbox dependencies * Fixes the depcheck
There was a problem hiding this 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], |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
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 |
I've merged in the |
"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", |
There was a problem hiding this comment.
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: { |
There was a problem hiding this comment.
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.
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
anddevDependencies
, bumping up most of them to the latest releases across all of ourpackage.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 thewebpack
-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.