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

Migrate Gulp and Node.js build tasks to ESM #2982

Merged
merged 3 commits into from
Nov 21, 2022
Merged

Migrate Gulp and Node.js build tasks to ESM #2982

merged 3 commits into from
Nov 21, 2022

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Nov 8, 2022 •
edited
Loading

This PR upgrades our build tooling to ESM using *.mjs files

Why? We have a couple of packages noted in #2937 with "ESM only" upgrades

Intentionally ignored until we use ESM by default

del → ^7.0.0 – ESM export only, we need CommonJS
slash → ^5.0.0 – ESM export only, we need CommonJS

Both of them are flagged by dependabot:

Additional considerations

The following Gulp tasks have been removed by name but are still part of their npm script:

gulp scss:compile → npm run compile
gulp js:compile → npm run compile
gulp copy:assets → npm run build:dist
gulp copy:files → npm run build:package

For convenience I've kept gulp watch but perhaps npm start is enough?

I've also ensured new ESM imports have no "side effects" which means moving (some) gulp.task() calls to gulpfile.mjs which chips away a bit more at:

@colinrotherham colinrotherham requested a review from a team November 8, 2022 13:23
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2982 November 8, 2022 13:23 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2982 November 8, 2022 13:52 Inactive
@domoscargin domoscargin self-requested a review November 8, 2022 13:54
@colinrotherham colinrotherham mentioned this pull request Nov 8, 2022
Merged
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2982 November 8, 2022 14:07 Inactive
colinrotherham
colinrotherham commented Nov 8, 2022
View reviewed changes
gulpfile.mjs Outdated
* Watch task
* Runs tasks when files are modified
*/
gulp.task('watch', watch)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this so gulp watch still works, do we want this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I say nah. Can't think of a good enough reason to use it outside of the dev task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @domoscargin I'll remove it, rebase, and get this merged 😊

colinrotherham
colinrotherham commented Nov 8, 2022
View reviewed changes
gulpfile.mjs
require('./tasks/gulp/compile-assets.js')
require('./tasks/gulp/copy-to-destination.js')
require('./tasks/gulp/watch.js')
import { compileJavaScripts, compileStylesheets } from './tasks/gulp/compile-assets.mjs'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we happy losing the gulp js:compile and gulp scss:compile tasks?

They're still available via gulp compile which includes compileJavaScripts() and compileStylesheets()

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see edge cases where somebody is working on some finicky interaction between CSS and JS, but that feels such an unlikely case, and these are not part of the "public" package json scripts, so I think we're OK to toss 'em.

colinrotherham
colinrotherham commented Nov 8, 2022
View reviewed changes
tasks/gulp/copy-to-destination.mjs
*
* @returns {import('stream').Stream} Output file stream
*/
export function copyAssets () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this one from gulpfile.mjs

Made sense to have copyAssets() (fonts, images) alongside copyFiles()

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2982 November 8, 2022 15:05 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2982 November 9, 2022 21:37 Inactive
@colinrotherham colinrotherham mentioned this pull request Nov 11, 2022
Closed
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2982 November 14, 2022 15:56 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2982 November 17, 2022 16:59 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2982 November 18, 2022 12:56 Inactive
domoscargin
domoscargin approved these changes Nov 21, 2022
View reviewed changes
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.

I don't have any blocking comments - I think this makes sense, especially since we're using ESM for most of our source code. Ran through the npm scripts and they seemed fine.

gulpfile.mjs
require('./tasks/gulp/compile-assets.js')
require('./tasks/gulp/copy-to-destination.js')
require('./tasks/gulp/watch.js')
import { compileJavaScripts, compileStylesheets } from './tasks/gulp/compile-assets.mjs'
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see edge cases where somebody is working on some finicky interaction between CSS and JS, but that feels such an unlikely case, and these are not part of the "public" package json scripts, so I think we're OK to toss 'em.

gulpfile.mjs Outdated
* Watch task
* Runs tasks when files are modified
*/
gulp.task('watch', 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 say nah. Can't think of a good enough reason to use it outside of the dev task.

@colinrotherham
Prefer yargs-parser versus yargs CLI tool
2fbdeeb 
Also avoids the named `yargs` ESM export differing to the destructured `{ yargs }` CommonJS export
@colinrotherham
Prefer minimatch to slash in PostCSS config
e72c9b0 
Avoids blocking a future upgrade to `slash@5` (ESM only) since we can’t yet use an ESM `postcss.config.mjs` without `postcss-load-config@4`

We’ll need to keep a CommonJS config for a little longer
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2982 November 21, 2022 17:43 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2982 November 21, 2022 17:51 Inactive
@colinrotherham colinrotherham merged commit ec5fa30 into main Nov 21, 2022
@colinrotherham colinrotherham deleted the gulp-esm branch November 21, 2022 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domoscargin domoscargin domoscargin approved these changes

Assignees
No one assigned
Labels
interoperability tooling
Projects
GOV.UK Design System cycle board
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@colinrotherham @domoscargin @govuk-design-system-ci