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

Fix problem with fsevents@1.2.7 and Node.js 12 #1663

Merged
merged 5 commits into from Nov 25, 2019

Conversation

colinrotherham
Copy link
Contributor

Updates:

  1. All non-breaking child dependencies.
  2. Some carefully-checked breaking changes

This pull fixes node-gyp build issues in #1662

I've also pinned postcss-pseudo-classes@0.2.0 as the 0.2.1 update throws an “Expected closing parenthesis” error due to the current blacklist for the :not() selector hack.

Breaking but not breaking changes
Also included because they're major semver bumps, but aren't breaking for GOV.UK Frontend:

acorn@^7.1.0
Raises default ES2018 to ES2019, supports dynamic import() from Node.js 10

gulp-if@^3.0.0
Now only supports Node.js 8+

jest-serializer-html@^7.0.0
Untrimmed whitespace now matters in matched HTML

nodemon@^2.0.1
Now only supports Node.js 8+ (including chokidar fixes for Node.js 12)

vinyl-paths@^3.0.1
Now only supports Node.js 8+

yargs@^15.0.2
Now only supports Node.js 8+
Throws errors for invalid arguments config

Pinning `postcss-pseudo-classes@0.2.0` due to “Expected closing parenthesis” blacklist config issue
@36degrees 36degrees added this to Needs review in Design System Sprint Board via automation Nov 25, 2019
@36degrees 36degrees removed this from Needs review in Design System Sprint Board Nov 25, 2019
@36degrees 36degrees self-requested a review November 25, 2019 08:59
Copy link
Member

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Thanks, @colinrotherham – really appreciate your work on this. This just needs a second set of eyes and then we can get this merged

@colinrotherham
Copy link
Contributor Author

No problem!

Also held back puppeteer@2.0.0 as we're waiting on argos-ci/jest-puppeteer#289 too

@NickColley NickColley self-assigned this Nov 25, 2019
@@ -5,7 +5,7 @@ const configPaths = require('../../config/paths.json')
// Watch task ----------------------------
// When a file is changed, re-run the build task.
// ---------------------------------------
gulp.task('watch', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe this change is necessary as it was a problem with my computer, sorry!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If an error does happen in watch in future you'll want the promise to reject and be fed back to gulp so it can handle it correctly. Sure you don't want to leave it in?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds useful to me so can leave it in sure.

Copy link
Contributor

@NickColley NickColley left a comment

Choose a reason for hiding this comment

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

Managed to get this running on my end by running killall node until there were not Node.js instances running.

Doesn’t need to watch for changes in ./package
@NickColley
Copy link
Contributor

Thanks for tidying this all up, very good improvements.

@NickColley NickColley merged commit 0577417 into alphagov:master Nov 25, 2019
@colinrotherham colinrotherham deleted the package-updates branch November 25, 2019 12:25
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.

None yet

3 participants