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 gulpfile from CommonJS to ECMAScript modules (ESM) #3181

Merged
merged 5 commits into from
Oct 29, 2022
Merged

Refactor gulpfile from CommonJS to ECMAScript modules (ESM) #3181

merged 5 commits into from
Oct 29, 2022

Conversation

MikeMcC399
Copy link
Contributor

@MikeMcC399 MikeMcC399 commented Oct 27, 2022

This PR refactors gulpfile.js, which builds the website, so that it uses an ECMAScript modules (ESM) structure. This is compatible with newer ESM packages and with older CommonJS packages.

It address issue #3176 "Update gulpfile to use ESM".

Changes

The gulpfile.js is renamed to gulpfile.mjs (see https://nodejs.org/docs/latest-v16.x/api/esm.html#enabling) so that ESM is automatically used.

The CommonJS require lines in the file which called in the necessary npm packages are converted to ESM import syntax (see https://nodejs.org/docs/latest-v16.x/api/esm.html#no-require-exports-or-moduleexports). In addition the order of the import lines has been sorted for better readability:

  1. The use of the gulp-load-plugins module is removed and replaced by individual imports of gulp modules. This is because gulp-load-plugins produces the error TypeError: Cannot read properties of undefined (reading 'filename') in an ESM environment.
    https://github.com/jackfranklin/gulp-load-plugins/releases/tag/v2.0.8 provides a workaround, however this does not seem reliable for the multiple environments which this web may be built under.

  2. The associated $. constructs are also replaced accordingly (gulp-if, gulp-imagemin, gulp-postcss, gulp-sitemap and gulp-sourcemaps). Several gulp modules were already separately required so the use of gulp-load-plugins was already inconsistently applied.

  3. mozjpeg called from gulp-imagemin needs special migration treatment. See https://github.com/sindresorhus/gulp-imagemin#custom-plugin-options for new syntax necessary for gulp-imagemin 8.0.0 (otherwise "TypeError: imagemin.mozjpeg is not a function" is output).

  4. The tag assert {type: 'json'} is added to json imports. This produces a warning in node 16: "ExperimentalWarning: Importing JSON modules is an experimental feature. This feature could change at any time".

Verification

Ensure that the web can be built using npm run build.

Execute npm install gulp-imagemin@latest to install version 8.0.0 and check again for a successful web build.
Restore the gulp-imagemin to version 7.1.0 with
git restore .
npm ci

Check at any time which version of gulp-imagemin is installed using npm ls gulp-imagemin.

Ensure that npm test successfully runs the Cypress test suite.


Internal Tracking ID: EXPOSUREAPP-14248

@dsarkar
Copy link
Member

dsarkar commented Oct 27, 2022

@MikeMcC399 Thank you very much for providing this PR. We will review it ASAP.

@mtb77 mtb77 self-requested a review October 27, 2022 15:51
@MikeMcC399
Copy link
Contributor Author

Successfully tested on Windows 11 and Ubuntu 20.04 with gulp-imagemin 7.1.0 and 8.0.0.

@MikeMcC399
Copy link
Contributor Author

  • Removed npm uninstall of gulp-load-plugins and gulp-uglify to avoid conflict with pending PR dependency Update to fix Nodev18 #3183. These two npm packages can be uninstalled separately later.

@MikeMcC399 MikeMcC399 marked this pull request as draft October 28, 2022 05:12
@MikeMcC399
Copy link
Contributor Author

MikeMcC399 commented Oct 28, 2022

Investigation needed, so setting to Draft status.

Updating yargs from the current "yargs": "^15.4.1" to "yargs": "^17.6.0" in PR #3183 is causing this incompatibility.

@MikeMcC399 MikeMcC399 marked this pull request as ready for review October 28, 2022 06:10
@MikeMcC399
Copy link
Contributor Author

An upgrade to yargs >= v16 could be looked at separately, however it is not necessary at this time.

@MikeMcC399
Copy link
Contributor Author

MikeMcC399 commented Oct 28, 2022

The answer to the yargs incompatibility is in https://stackoverflow.com/questions/65712177/im-getting-yargs-methodname-is-not-a-function-with-all-yargs-methods-using-es6

By changing the import of yargs in gulpfile.mjs to

import npmYargs from 'yargs';
import { hideBin } from 'yargs/helpers';
const yargs = npmYargs(hideBin(process.argv));

npm run build then works with yargs@16 and yargs@17, however it is not backwards compatible with yargs@15 (current version in master branch), so I have left this PR compatible with yargs@15. This could be changed with a later PR which updates both gulpfile.mjs and the version of yargs in package.json to v17 in the same PR.

@Ein-Tim
Copy link
Contributor

Ein-Tim commented Oct 28, 2022

Please let me know if I should test this PR on a M1 mac.

@MikeMcC399
Copy link
Contributor Author

@Ein-Tim

Please let me know if I should test this PR on a M1 mac.

Thank you for your offer! You are welcome to test it in your environment.

Copy link
Contributor

@Ein-Tim Ein-Tim left a comment

Choose a reason for hiding this comment

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

Tested on a M1 MacBook Air. After I restarted my machine, it worked. So nothing is blocking this PR from being merged IMO.

@MikeMcC399
Copy link
Contributor Author

@Ein-Tim

Tested on a M1 MacBook Air. After I restarted my machine, it worked. So nothing is blocking this PR from being merged IMO.

I much appreciate your testing on M1. I already tested on Ubuntu and Windows 11, so I also think this PR is fine.

@thomasaugsten thomasaugsten merged commit 14346df into corona-warn-app:master Oct 29, 2022
@MikeMcC399 MikeMcC399 deleted the update/gulp-to-esm branch October 30, 2022 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants