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.

Sign up for GitHub

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

Jump to bottom

Remove gulp functionality #1324

Merged
merged 7 commits into from
May 23, 2022
Merged

Remove gulp functionality #1324

merged 7 commits into from
May 23, 2022

Conversation

BenSurgisonGDS
Copy link
Contributor

This PR resolves issue: #1290

Changes:

  • Replaced gulp-watch with chokidar
  • Replaced gulp-nodemon with nonemon
  • Replaced gulp-sass with sass
  • Removed gulp from the npm test script
  • Removed all references to gulp including gulp itself, the gulpfile and the gulp folder
  • Added a build folder including run-tasks to replace the previous gulp functionality above
  • Amended the jest tests for the above
  • Updated the update.sh bash script for the above

@lfdebrux
Copy link
Member

lfdebrux commented May 19, 2022 •
edited
Loading

Looking good! I'm going to clean up the history of these commits a bit, to try and split out the changes related to docs and v6 into separate commits.

Edited to add: Done :)

@lfdebrux lfdebrux force-pushed the remove-gulp-functionality branch from c57b431 to 95bf7f2 Compare May 19, 2022 14:26
@lfdebrux
Copy link
Member

Look at all those deletions 🥲

% git diff --stat main package-lock.json
 package-lock.json | 6648 ++++++++++++-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 1 file changed, 387 insertions(+), 6261 deletions(-)

@lfdebrux lfdebrux force-pushed the remove-gulp-functionality branch from 95bf7f2 to 5ba0b9a Compare May 19, 2022 14:29
@lfdebrux
Copy link
Member

I'd like us to consider putting the build stuff in the lib folder, to keep it out of sight from users. We could maybe work towards putting everything in there as sort of a halfway-house towards having a proper package... what do you think @BenSurgisonGDS @natcarey?

lfdebrux
lfdebrux reviewed May 19, 2022
View reviewed changes
lfdebrux
lfdebrux reviewed May 20, 2022
View reviewed changes
lfdebrux
lfdebrux reviewed May 20, 2022
View reviewed changes
lfdebrux
lfdebrux reviewed May 20, 2022
View reviewed changes
__tests__/spec/sanity-checks.js

describe(`${gulpConfig.paths.assets}sass/`, () => {
describe(`${buildConfig.paths.assets}sass/`, () => {
it.each(sassFiles)('%s renders to CSS without errors', async (file) => {
return new Promise((resolve, reject) => {
sass.render({
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we can change this line to use a function from build/run-tasks.js... that feels like it would be a better test?

BenSurgisonGDS and others added 5 commits May 20, 2022 16:51
@BenSurgisonGDS @joelanman
Replace gulp with plain old Node.js code
6b00c53 
We want to avoid using gulp, as it has a load of dependencies. With
better fs libraries in Node.js we can do this.

Co-authored-by: Ben Surgison <ben.surgison@digital.cabinet-office.gov.uk>
Co-authored-by: Joe Lanman <joelanman@gmail.com>
@BenSurgisonGDS
Fix tests on Node 12
af377c6 
Node 12 and below don't have the `fs.rmSync` or `fs.rm` functions, and
has limited support for removing a directory recursively. Instead we
use `del`, which we include as a dependency anyway.
@lfdebrux @BenSurgisonGDS
Fix Sass compilation in tests
cbbae7f 
There is a bug where `sass.compile` will throw with an extremely
unhelpful error trace when running in Jest [[1]].

>     TypeError: J.getInterceptor$ax(...).map$1$1 is not a function
>
>     at Object.map$1$1$ax (node_modules/sass/sass.dart.js:24513:44)
>     at listDir_closure0.call$0 (node_modules/sass/sass.dart.js:87066:18)
>     at Object._systemErrorToFileSystemException0 (node_modules/sass/sass.dart.js:20776:23)
>     at Object.listDir0 (node_modules/sass/sass.dart.js:20771:16)
>     at _realCasePath_helper_closure0.call$0 (node_modules/sass/sass.dart.js:84710:34)
>     at JsLinkedHashMap.putIfAbsent$2 (node_modules/sass/sass.dart.js:27273:24)
>     at _realCasePath_helper0.call$1 (node_modules/sass/sass.dart.js:84699:32)
>     at _realCasePath_helper_closure0.call$0 (node_modules/sass/sass.dart.js:84706:35)
>     at JsLinkedHashMap.putIfAbsent$2 (node_modules/sass/sass.dart.js:27273:24)
>     at _realCasePath_helper0.call$1 (node_modules/sass/sass.dart.js:84699:32)

It has been suggested
that it is related to `path.absolute` being overridden by the jsdom
environment [[2]], but the workaround of using the "node" environment
does not solve the issue for us.

Instead, what did work is using the legacy API [[3]]. I'm not sure why
this is, but it seems to do the trick. Note that with the legacy API,
the file path is part of the options object, rather than being the
first argument. If you forget to change the call signature, you will get
a different unhelpful error message:

> NoSuchMethodError: method not found: 'call'

[1]: sass/dart-sass#1692
[2]: sass/dart-sass#710 (comment)
[3]: https://sass-lang.com/documentation/js-api#legacy-api
@BenSurgisonGDS
Ensure that update script removes gulp files
9dae02a
@BenSurgisonGDS
Also handle v6 and docs
3c3f3a2
@BenSurgisonGDS BenSurgisonGDS force-pushed the remove-gulp-functionality branch from 5ba0b9a to 3c3f3a2 Compare May 20, 2022 15:52
joelanman
joelanman reviewed May 20, 2022
View reviewed changes
lib/build/run-tasks.js Outdated
}

function clean () {
// doesn't clean extensions.scss, should it?
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably resolve this question @natcarey

joelanman
joelanman reviewed May 20, 2022
View reviewed changes
@BenSurgisonGDS BenSurgisonGDS force-pushed the remove-gulp-functionality branch from f3a4696 to 515a76b Compare May 20, 2022 17:05
@BenSurgisonGDS BenSurgisonGDS force-pushed the remove-gulp-functionality branch from e01f998 to f07b196 Compare May 20, 2022 17:14
lfdebrux
lfdebrux approved these changes May 23, 2022
View reviewed changes
Copy link
Member

@lfdebrux lfdebrux left a comment

Choose a reason for hiding this comment

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

Looks good to me

@BenSurgisonGDS BenSurgisonGDS merged commit c8aba1e into main May 23, 2022
@BenSurgisonGDS BenSurgisonGDS deleted the remove-gulp-functionality branch May 23, 2022 10:49
@lfdebrux lfdebrux mentioned this pull request May 24, 2022
Closed
@domoscargin domoscargin mentioned this pull request Jun 29, 2022
Closed
4 tasks
@joelanman joelanman mentioned this pull request Aug 17, 2022
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelanman joelanman joelanman left review comments

@lfdebrux lfdebrux lfdebrux approved these changes

@nataliecarey nataliecarey Awaiting requested review from nataliecarey

Assignees

@lfdebrux lfdebrux

@BenSurgisonGDS BenSurgisonGDS

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@BenSurgisonGDS @lfdebrux @joelanman