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

Refactor asset-version build task to remove Gulp dependency #2763

Merged
merged 3 commits into from
Aug 15, 2022

Conversation

querkmachine
Copy link
Member

@querkmachine querkmachine commented Aug 12, 2022

Removes the Gulp dependency from the asset-version build task. Resolves #2715.

Specifically, this PR:

  • Rewrites the update-assets-version Gulp task to be an exported JavaScript function, renamed as updateDistAssetsVersion.
  • Moves asset-version.js to the tasks directory.
  • Removes dependency on Gulp and Gulp plugins from the task.
  • Removes use of the destination variable from task-arguments.js to determine where compiled files go, as per this comment. This task only ever seems to compile to dist, so this has been hardcoded.
  • Removes the vinyl-paths package, as this is no longer used anywhere.
  • Modifies gulpfile.js to integrate the renamed task.

Related changes:

  • Moves task-arguments.js to the tasks directory.
  • Modifies the paths in some unrelated task files to account for moving task-arguments.js.

These changes appear to produce the expected compiled files, filenames, and passes our automated tests.

Note: The updateDistAssetsVersion function is required to accept and return a callback function so that it can be integrated with the existing Gulp tasks. This could be removed once we've gotten rid of Gulp entirely.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2763 August 12, 2022 13:47 Inactive
@querkmachine querkmachine requested a review from a team August 12, 2022 13:48
Copy link
Contributor

@domoscargin domoscargin left a comment

Choose a reason for hiding this comment

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

Looking good. This seems to produce the same dist folder and files for me.

A coupla comments with some reckons but very excited to be leaving Gulp behind!

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2763 August 15, 2022 09:07 Inactive
@querkmachine querkmachine force-pushed the kg-remove-gulp-asset-version branch from 9966469 to 0f68168 Compare August 15, 2022 10:16
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2763 August 15, 2022 10:16 Inactive
Copy link
Contributor

@domoscargin domoscargin left a comment

Choose a reason for hiding this comment

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

🌲 Breathe the sweet air of this post-gulp world! 🌳

Nice work!

@querkmachine querkmachine merged commit 8f1f351 into main Aug 15, 2022
@querkmachine querkmachine deleted the kg-remove-gulp-asset-version branch August 15, 2022 10:33
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.

Refactor asset version task to remove Gulp
3 participants