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

Add missing mandatory file extensions for ESM JavaScript #2658

Merged
merged 4 commits into from Jun 20, 2022
Merged

Add missing mandatory file extensions for ESM JavaScript #2658

merged 4 commits into from Jun 20, 2022

Conversation

vanitabarrett
Copy link
Contributor

@vanitabarrett vanitabarrett commented Jun 10, 2022 •
edited

Fixes #2633

A file extension must be provided when using the import keyword to resolve relative or absolute specifiers. Directory indexes (e.g. './startup/index.js') must also be fully specified.
https://nodejs.org/api/esm.html#mandatory-file-extensions

This PR has been updated to go with Option 2, where our JavaScript files are .mjs by default.

Two approaches here at the moment:

  • Option 1: minimal changes to the pre-built files, just adding the missing .js file extensions. This means we need to update these file extensions when we copy the files in package/govuk-esm/ so that they reference .mjs rather than .js
  • Option 2: pre-built files use .mjs as they're already in ESM format. These can be copied directly over to package/govuk-esm/ with no changes. UMD build (using rollup) gets rid of (resolves) import statements anyway and transform to .js.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2658 June 10, 2022 12:57 Inactive
@domoscargin
Copy link
Contributor

Option 2 feels neater to me and I guess it's more "correct" since the source code is esm. Plus I can just imagine some edge case accident with the string replacement in the first option biting us.

It works locally for me for various NPM scripts, and the output of dist/govuk-frontend-4.1.0.min.js and (as an example) package/govuk/components/accordion/accordion.js are exactly the same between main and this PR.

One possibly overboard thought: do we want to protect against Rollup bugs by confirming there's no imports in the UMD or something? Nah, seems like the test would probably be more prone to bugs than the Rollup process!

@36degrees
Copy link
Member

Agreed that option 2 seems like the more 'honest' approach, and also puts us on the right track to eventually move entirely to ESM.

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2658 June 14, 2022 15:13 Inactive
@vanitabarrett vanitabarrett mentioned this pull request Jun 14, 2022
Closed
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2658 June 14, 2022 15:38 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2658 June 14, 2022 15:44 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2658 June 14, 2022 15:48 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2658 June 15, 2022 10:53 Inactive
@vanitabarrett vanitabarrett changed the title [WIP][DO NOT MERGE] Fix ESM error with missing mandatory file extensions. Fix ESM error with missing mandatory file extensions. Jun 15, 2022
@vanitabarrett vanitabarrett changed the title Fix ESM error with missing mandatory file extensions. Fix ESM error with missing mandatory file extensions Jun 15, 2022
@vanitabarrett vanitabarrett changed the title Fix ESM error with missing mandatory file extensions Add missing mandatory file extensions for ESM JavaScript Jun 15, 2022
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2658 June 15, 2022 14:48 Inactive
domoscargin
domoscargin reviewed Jun 16, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
domoscargin added a commit to alphagov/govuk-frontend-docs that referenced this pull request Jun 16, 2022
@domoscargin
Remove unnecessary guidance for webpack
36266fa 
We added this guidance for govuk-frontend v4.1.0 - when we added the ability to use ES Modules - because our imports were not fully specified (ie: they did not have file extensions). We've [fixed this for v4.1.1](alphagov/govuk-frontend#2658), and this guidance now looks to be unneeded.

Part of #194
@domoscargin domoscargin mentioned this pull request Jun 16, 2022
Merged
domoscargin added a commit to alphagov/govuk-frontend-docs that referenced this pull request Jun 16, 2022
@domoscargin
Remove unnecessary guidance for webpack
ae5f82c 
We added this guidance for govuk-frontend v4.1.0 - when we added the ability to use ES Modules - because our imports were not fully specified (ie: they did not have file extensions). We've [fixed this for v4.1.1](alphagov/govuk-frontend#2658), and this guidance now looks to be unneeded.

Part of #194
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2658 June 20, 2022 09:09 Inactive
@vanitabarrett vanitabarrett marked this pull request as ready for review June 20, 2022 09:09
@vanitabarrett vanitabarrett requested a review from a team June 20, 2022 09:09
@domoscargin
Copy link
Contributor

Small update needed in the Gulp Tasks doc (.js to .mjs)

govuk-frontend/docs/contributing/tasks.md

Line 62 in da32bb0

- watch for changes in .js, .scss and .njk files and run below tasks.

domoscargin
domoscargin approved these changes Jun 20, 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've left a very minor comment, but otherwise this all looks good to me, and still works as expected locally. 🎉

@vanitabarrett
Copy link
Contributor Author

Great spot @domoscargin , thanks! Will change that now

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2658 June 20, 2022 15:55 Inactive
Make all JavaScript files .mjs by default, and transform to .js using…
9d4e438 
… rollup

Allows us to add file extensions to the end of our JavaScript imports, which means
our ESM code has "fully specified" import paths as required:

A file extension must be provided when using the import keyword to resolve relative or absolute specifiers. Directory indexes (e.g. './startup/index.js') must also be fully specified.
https://nodejs.org/api/esm.html#mandatory-file-extensions
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2658 June 20, 2022 16:00 Inactive
@vanitabarrett vanitabarrett merged commit 9a091c3 into main Jun 20, 2022
@vanitabarrett vanitabarrett deleted the full-resolve-js-path branch June 20, 2022 16:07
@domoscargin domoscargin mentioned this pull request Jun 27, 2022
Merged
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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ES Modules can't be imported without a bundler
4 participants
@vanitabarrett @domoscargin @36degrees @govuk-design-system-ci