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

Rename exported JavaScript modules to include component name #2426

Merged
merged 5 commits into from
Nov 19, 2021

Conversation

vanitabarrett
Copy link
Contributor

@vanitabarrett vanitabarrett commented Nov 9, 2021

Closes #1836

What

Change the window global and UMD export name for component JavaScript to include the component name, for example: GOVUKFrontend.CharacterCount.

The way this PR currently stands, this new naming applies only to component JavaScript files - not toall.js, common.js or any vendor/polyfills. Because all.js already exports multiple modules, we can keep the original naming of GOVUKFrontend and modules will already be automatically nested underneath, e.g: window.GOVUKFrontend.initAll(). If the new naming system was also applied to all.js, you would end up having to call window.GOVUKFrontend.All.initAll().

This means that if a user imports a specific component JavaScript (e.g: character-count) and thenall.js - all.js will overwrite any existing modules attached to window.GOVUKFrontend. This feels like an unlikely scenario, but I'm not sure I know enough about how users are importing our JavaScript to know if applying this change only to component JavaScript is likely to cause problems - welcome any thoughts/insights on this.

Why

When importing individual scripts, each one was attached to window.GOVUKFrontend. This meant that if you imported (for example) the accordion and then imported the button JavaScript, one overwrote the other so that only the button JavaScript would be attached to window.GOVUKFrontend.

Breaking Changes

Again - I welcome any thoughts/insights on this as I'm not 100% confident I know enough about how users are using our JavaScript to account for all changes that might need to be made

If a user is importing all our JavaScript, I don't think they'll need to make any changes.

If a user is importing individual component JavaScript, then they will need to switch any calls to window.GOVUKFrontend to be to window.GOVUKFrontend.[ComponentName]

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2426 November 9, 2021 11:54 Inactive
package.json Outdated Show resolved Hide resolved
@lfdebrux
Copy link
Member

lfdebrux commented Nov 9, 2021

I'm not sure I understand what is going on in the first commit... would you mind expanding a bit on why you're making the change and what it means? 😅

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2426 November 9, 2021 13:30 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2426 November 9, 2021 13:42 Inactive
@vanitabarrett
Copy link
Contributor Author

@lfdebrux I'll try to explain it more here, let me know if it's any more helpful! If not, happy to jump on a call to chat about it.

The current gulp task as it is on main sets up an array of JavaScript files, and gulp uses streams to pass that data through the rest of the transformations (rollup, uglify etc). We need to make a change to the rollup config so that the name contains the component name and isn't just "GOVUKFrontend" for every single module. In order to do that, we need some way of accessing what current file we're "on"/"processing" so we can pull out the relevant component name.

As far as I can tell, there's no easy way of doing this with the current set-up as gulp streams mean you don't get that concept of 'this is the thing that's currently being processed'. I might be wrong about that, welcome to other suggestions!

So in order to get more "control" over knowing what file is currently being processed, I've introduced a forEach loop which loops through every JavaScript file and passes that through the same gulp process as before - but this time, gulp is being passed one file at a time rather than an array of files at the start. That means we know what file we're on and we have access to the pathname and filename etc.

@lfdebrux
Copy link
Member

lfdebrux commented Nov 9, 2021

@lfdebrux I'll try to explain it more here, let me know if it's any more helpful! If not, happy to jump on a call to chat about it.

The current gulp task as it is on main sets up an array of JavaScript files, and gulp uses streams to pass that data through the rest of the transformations (rollup, uglify etc). We need to make a change to the rollup config so that the name contains the component name and isn't just "GOVUKFrontend" for every single module. In order to do that, we need some way of accessing what current file we're "on"/"processing" so we can pull out the relevant component name.

As far as I can tell, there's no easy way of doing this with the current set-up as gulp streams mean you don't get that concept of 'this is the thing that's currently being processed'. I might be wrong about that, welcome to other suggestions!

So in order to get more "control" over knowing what file is currently being processed, I've introduced a forEach loop which loops through every JavaScript file and passes that through the same gulp process as before - but this time, gulp is being passed one file at a time rather than an array of files at the start. That means we know what file we're on and we have access to the pathname and filename etc.

Ah, I get it now, thank you!

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2426 November 12, 2021 13:47 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2426 November 12, 2021 13:50 Inactive
@vanitabarrett vanitabarrett marked this pull request as ready for review November 12, 2021 14:08
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

It'd be nice to have tests to cover at least some of this, but no idea how to make that happen…

Maybe we could add a test to after-build-package.test.js that looks at the component JS and asserts something about the exported name?

tasks/gulp/compile-assets.js Outdated Show resolved Hide resolved
tasks/gulp/compile-assets.js Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2426 November 15, 2021 14:28 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2426 November 15, 2021 17:09 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2426 November 15, 2021 17:13 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2426 November 16, 2021 09:59 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2426 November 16, 2021 10:05 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2426 November 16, 2021 11:07 Inactive
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Looks good to me, just one suggestion re how we import the componentNameToJavaScriptModuleName method.

tasks/gulp/__tests__/after-build-package.test.js Outdated Show resolved Hide resolved
tasks/gulp/compile-assets.js Outdated Show resolved Hide resolved
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2426 November 16, 2021 11:33 Inactive
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Have manually run the build:dist and build:package tasks and the output looks like what I'd expect – although it looks like using the dist version of the JS the modules are not attached to GOVUKFrontend, but I guess that's expected? It doesn't appear to be a change from the existing behaviour, anyway.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@EoinShaughnessy EoinShaughnessy left a comment

Choose a reason for hiding this comment

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

@vanitabarrett Looks great! Just a few tiny comments.

Copy link
Contributor

@EoinShaughnessy EoinShaughnessy left a comment

Choose a reason for hiding this comment

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

@vanitabarrett Looks good!

@vanitabarrett
Copy link
Contributor Author

Thanks for your reviews @EoinShaughnessy and @36degrees ! I'm going to hold off merging this for a bit as I've asked @alex-ju to test this change in the GOV.UK Publishing Components to double check it works as expected.

alex-ju added a commit to alphagov/govuk_publishing_components that referenced this pull request Nov 18, 2021
Test alphagov/govuk-frontend#2426 ,a prerelease using `yarn add alphagov/govuk-frontend#00603a9b`
@alex-ju
Copy link
Contributor

alex-ju commented Nov 18, 2021

Many thanks for the work on this, Vanita! 🙌
Tested this change in alphagov/govuk_publishing_components#2457 and it works as expected.
This approach – with window.GOVUKFrontend.initAll() (when importing everything) and window.GOVUKFrontend.Button(element).init() (for individual imports) – is what I would expect as a consumer.

Vanita Barrett added 5 commits November 19, 2021 09:27
Changes the existing JS compile gulp task to use `glob` and iterate over
each file using `forEach`.
Add a new function to our helper-functions.js that
converts a component name to the Javascript module name.

This means we can use the function elsewhere, such as tests, without
duplicating the logic. It also makes sense to put together with the function
that generates the macro names, as the two are linked
Adds the module name (e.g: Accordion) to the UMD export name.
For example: GOVUKFrontend becomes GOVUKFrontend.Accordion

If the file is all.js (which pulls in all component JS), fall back to the
original naming ("GOVUKFrontend") otherwise the modules are nested underneath
an `all`, e.g: GOVUKFrontend.all.initAll()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JavaScript modules are attached to the same global variable when imported individually
6 participants