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

Don’t copy *.js files into govuk-esm #2876

Merged
merged 3 commits into from
Sep 26, 2022
Merged

Conversation

colinrotherham
Copy link
Contributor

In response to comment regarding *.js files in govuk-esm

So I think we can just remove the ${configPaths.src}**/*.js glob from the js:copy-esm task?

This PR includes:

  1. Combine js:copy-esm task → copy-files
  2. Remove ${configPaths.src}**/*.js glob so only *.mjs are copied

@@ -15,54 +15,61 @@ const taskArguments = require('../task-arguments')

gulp.task('copy-files', () => {
return merge(
// Copy source JavaScript
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Copy source JavaScript
// Copy ESM files to make them available in the package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I suggest a tweak to align with the Gulpfile task?

// Prepare package folder for publishing

At the moment we use --destination to choose ./package so the destination could vary

Suggested change
// Copy source JavaScript
// Copy ESM JavaScript to destination, ready for publishing

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 @romaricpascal

I've added a new commit with better comments for all these lines (and more!)

// https://github.com/alphagov/govuk-frontend/tree/main/package#readme
`!${configPaths.src}README.md`,

// Exclude files from other streams
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Exclude files from other streams
// Exclude files from other streams (MJS and test files are already excluded a bit above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@romaricpascal This one definitely needed a better comment

It was actually excluding files handled by the PostCSS and JSON (from YAML) streams a bit below

Hopefully this is clearer:

// Exclude Sass files handled by PostCSS stream below
`!${configPaths.src}**/*.scss`,

// Exclude source YAML handled by JSON streams below
`!${configPaths.components}**/*.yaml`

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.

It looks good to me 🙌🏻 Just got two little comments to clarify what tripped me up while wrapping my head around things. Feel free to reword or disregard if they're not relevant 😄

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2876 September 26, 2022 11:21 Inactive
@colinrotherham
Copy link
Contributor Author

It looks good to me 🙌🏻 Just got two little comments to clarify what tripped me up while wrapping my head around things. Feel free to reword or disregard if they're not relevant 😄

Thanks, see what you think now

@colinrotherham colinrotherham merged commit 5369f8e into main Sep 26, 2022
@colinrotherham colinrotherham deleted the source-javascript-copy branch September 26, 2022 13:16
@colinrotherham
Copy link
Contributor Author

This PR was related to #2243

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

3 participants