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

Compile ES modules using Rollup #3386

Merged
merged 2 commits into from
Apr 3, 2023
Merged

Compile ES modules using Rollup #3386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b1af2b3
Compile ES modules using Rollup
colinrotherham Mar 15, 2023
f8a3a20
Prefer “UMD” in documentation (CommonJS, AMD compatible)
colinrotherham Mar 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/contributing/coding-standards/js.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ export function exampleHelper2 () {}

You must specify the file extension when using the import keyword.

Avoid using namespace imports (`import * as namespace`) in code transpiled to CommonJS (or AMD) bundled code as this can prevent "tree shaking" optimisations.
Avoid using namespace imports (`import * as namespace`) in code transpiled to UMD (or AMD, CommonJS) bundled code as this can prevent "tree shaking" optimisations.

Prefer named exports over default exports to avoid compatibility issues with transpiler "synthetic default" as discussed in: https://github.com/alphagov/govuk-frontend/issues/2829

Expand Down
17 changes: 7 additions & 10 deletions docs/contributing/tasks.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,9 @@ npm scripts are defined in `package.json`. These trigger a number of Gulp tasks.

**`npm start` will trigger `npm run dev --workspace app` that will:**

- clean the `./app/dist` folder
- copy fonts and images
- compile JavaScript and Sass, including documentation
- compile again when `.scss` and `.mjs` files change
- runs `npm run serve --workspace app`
- runs tasks from `npm run build:app`
- starts up Express, restarting when `.mjs`, `.json` or `.yaml` files change
- compile again when frontend `.mjs` and `.scss` files change

**`npm test` will do the following:**

Expand All @@ -31,8 +29,7 @@ npm scripts are defined in `package.json`. These trigger a number of Gulp tasks.
- clean the `./app/dist` folder
- output files into `./app/dist`
- copy fonts and images
- compile Sass to CSS, including documentation
- compile JavaScript ESM to CommonJS, including documentation
- compile JavaScript and Sass, including documentation

**`npm run build:package` will do the following:**

Expand All @@ -41,8 +38,8 @@ npm scripts are defined in `package.json`. These trigger a number of Gulp tasks.
- copy Sass files, applying Autoprefixer via PostCSS
- copy Nunjucks component template/macro files, including JSON configs
- copy GOV.UK Prototype Kit config files
- copy JavaScript ESM source files
- compile JavaScript ESM to CommonJS
- compile JavaScript to ECMAScript Modules (ESM)
- compile JavaScript to Universal Module Definition (UMD)
- runs `npm run postbuild:package` (which will test the output is correct)

**`npm run build:dist` will do the following:**
Expand Down Expand Up @@ -81,7 +78,7 @@ This task will:
This task will:

- check JavaScript code quality via ESLint (`npm run lint:js`) (using JavaScript Standard Style)
- compile JavaScript ESM to CommonJS into `./app/dist/javascripts`
- compile JavaScript to Universal Module Definition (UMD) into `./app/dist/javascripts`
- compile JavaScript documentation into `./app/dist/docs/jsdoc`

## Express app only
Expand Down
6 changes: 3 additions & 3 deletions tasks/build/package.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ export default gulp.series(
})
),

// Copy GOV.UK Frontend JavaScript (ES modules)
task.name('copy:mjs', () =>
files.copy('**/!(*.test).mjs', {
// Compile GOV.UK Frontend JavaScript (ES modules)
task.name('compile:mjs', () =>
scripts.compile('!(*.test).mjs', {
srcPath: join(paths.src, 'govuk'),
destPath: join(paths.package, 'govuk-esm')
})
Expand Down
15 changes: 10 additions & 5 deletions tasks/build/package.test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,14 @@ describe('package/', () => {
join(requirePath, `${name}.js.map`) // with source map
]

// Only source `./govuk/**/*.mjs` files copied to `./govuk-esm/**/*.mjs`
// Only source `./govuk/**/*.mjs` files compiled to `./govuk-esm/**/*.mjs`
if (importFilter.test(requirePath)) {
output.push(join(requirePath.replace(importFilter, 'govuk-esm'), `${name}.mjs`))
const importPath = requirePath.replace(importFilter, 'govuk-esm')

output.push(...[
join(importPath, `${name}.mjs`),
join(importPath, `${name}.mjs.map`) // with source map
])
}

return output
Expand Down Expand Up @@ -168,15 +173,15 @@ describe('package/', () => {
const componentPackage = componentsFilesPackage.filter(componentFilter)
const componentPackageESM = componentsFilesPackageESM.filter(componentFilter)

// CommonJS module not found at source
// UMD module not found at source
expect(componentSource)
.toEqual(expect.not.arrayContaining([join(componentName, `${componentName}.js`)]))

// CommonJS generated in package
// UMD module generated in package
expect(componentPackage)
.toEqual(expect.arrayContaining([join(componentName, `${componentName}.js`)]))

// ESM module generated in package
// ES module generated in package
expect(componentsFilesPackageESM)
.toEqual(expect.arrayContaining([join(componentName, `${componentName}.mjs`)]))

Expand Down
30 changes: 22 additions & 8 deletions tasks/scripts.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { componentPathToModuleName } from '../lib/helper-functions.js'
import { assets } from './index.mjs'

/**
* Compile JavaScript ESM to CommonJS task
* Compile JavaScript task
*
* @param {string} pattern - Minimatch pattern
* @param {AssetEntry[1]} [options] - Asset options
Expand All @@ -30,20 +30,34 @@ export async function compile (pattern, options) {
}

/**
* Compile JavaScript ESM to CommonJS helper
* Compile JavaScript helper
*
* @param {AssetEntry} assetEntry - Asset entry
*/
export async function compileJavaScript ([modulePath, { srcPath, destPath, filePath }]) {
const moduleSrcPath = join(srcPath, modulePath)
const moduleDestPath = join(destPath, filePath ? filePath(parse(modulePath)) : modulePath)

// Create Rollup bundle
const bundle = await rollup({
input: moduleSrcPath
})
// Option 1: Rollup bundle set (multiple files)
// - Module imports are preserved, not concatenated
if (moduleDestPath.endsWith('.mjs')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function seems to be doing 2 different jobs in two separate code paths – would it make sense to split it into two functions (one for generating a bundle and one that preserves modules) and decide which one to call in the compile function instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah could do. Only reason I didn't is that we'll just use the new code path in rollup@3 and delete the old

In newer versions of Rollup the { preserveModules } option can also output CJS/AMD

Is it a blocker?

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, so once we've upgraded to v3 we'll be able to simplify this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly. In this PR we process:

  1. Entry point *.mjs files for ESM, top level only
  2. Every non-test *.mjs file for UMD, recursively with **
task.name('compile:ms', () => scripts.compile('!(*.test).mjs') // Entries all.mjs, common.mjs etc
task.name('compile:js', () => scripts.compile('**/!(*.test).mjs') // Every non-test *.mjs file

But in future we'll use entry points for both and let Rollup discover all child modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, that makes sense 👍🏻 Happy to leave this for now then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We tried this in #3462 but we'd have to output CommonJS rather than the hybrid Universal Module Definition (UMD) bundles for browsers (see comment)

const bundle = await rollup({ input: [moduleSrcPath], experimentalPreserveModules: true })

// Compile JavaScript to ES modules
await bundle.write({
dir: destPath,
format: 'es',
sourcemap: true
})

return
}

// Option 1: Rollup bundle (single file)
// - Universal Module Definition (UMD) bundle
const bundle = await rollup({ input: moduleSrcPath })

// Compile JavaScript ESM to CommonJS
// Compile JavaScript to output format
const bundled = await bundle[moduleDestPath.endsWith('.min.js') ? 'generate' : 'write']({
file: moduleDestPath,
sourcemapFile: moduleDestPath,
Expand Down Expand Up @@ -72,7 +86,7 @@ export async function compileJavaScript ([modulePath, { srcPath, destPath, fileP
}

/**
* Minify JavaScript ESM to CommonJS helper
* Minify JavaScript helper
*
* @param {string} modulePath - Relative path to module
* @param {import('rollup').OutputChunk} result - Generated bundle
Expand Down