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

Use standalone Rollup task (no Gulp) #3088

Merged
merged 8 commits into from
Dec 20, 2022
Merged

Use standalone Rollup task (no Gulp) #3088

merged 8 commits into from
Dec 20, 2022

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Dec 9, 2022

Friday tech debt PR

This PR moves our Rollup compile:js task into a separate file but also:

  1. Removes all "gulp wrapper" legacy packages
  2. Calls Rollup and UglifyJS via their JavaScript API (no Gulp)
  3. Prepares us for Rollup v3 once we drop IE8

Also spotted for local development we were compiling all JavaScript files but only loading all.js

<script src="/public/all.js"></script>

That's now faster, plus we can remove some super old packages:

gulp-better-rollup
gulp-if
gulp-uglify

Ticks off some tasks from:

@colinrotherham colinrotherham force-pushed the rollup-standalone branch 7 times, most recently from e0ed994 to 16c4c00 Compare December 13, 2022 10:10
@romaricpascal romaricpascal self-assigned this Dec 15, 2022
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3088 December 15, 2022 11:21 Inactive
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

Looking pretty neat! 🙌🏻 Was surprised to not see a rollup.config.js file popping up to be directly used by the CLI and instead have it in our own tasks. But it does do the job 😄

I think it's been a little overreaching in removing the asset-version task though, the separation of concern by having it on its own was pretty clean.

tasks/compile-javascripts.mjs Outdated Show resolved Hide resolved
? 'all.mjs'
: '**/!(*.test).mjs'
// but for 'dist' and 'public' we only want compiled 'all.js'
const modulePaths = await getListing(srcPath, isPackage
Copy link
Member

Choose a reason for hiding this comment

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

Neat little update 🙌🏻

tasks/compile-javascripts.mjs Show resolved Hide resolved
config/paths.js Outdated Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3088 December 16, 2022 16:58 Inactive
@colinrotherham colinrotherham changed the base branch from main to pkg-ports-config December 16, 2022 16:58
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3088 December 16, 2022 17:08 Inactive
@colinrotherham
Copy link
Contributor Author

@romaricpascal This is ready for review again

Split out a couple of unrelated changes:

Base automatically changed from pkg-ports-config to main December 19, 2022 09:34
lib/helper-functions.js Show resolved Hide resolved
lib/helper-functions.js Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json.unit.test.js Outdated Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3088 December 20, 2022 11:07 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3088 December 20, 2022 11:11 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3088 December 20, 2022 11:40 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3088 December 20, 2022 11:49 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3088 December 20, 2022 11:56 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3088 December 20, 2022 13:51 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.

🔥

Thanks for popping in the tests - they're great! Getting rid of old gulp plugins is great! This is great!

@colinrotherham colinrotherham merged commit ee794e0 into main Dec 20, 2022
@colinrotherham colinrotherham deleted the rollup-standalone branch December 20, 2022 14:34
@colinrotherham colinrotherham added dependencies Pull requests that update a dependency file and removed tech debt labels Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file tooling
Projects
Development

Successfully merging this pull request may close these issues.

4 participants