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

[webpack] App manager tests #35662

Merged
merged 4 commits into from
Jan 23, 2025
Merged

[webpack] App manager tests #35662

merged 4 commits into from
Jan 23, 2025

Conversation

orangejenny
Copy link
Contributor

Safety Assurance

Safety story

This is largely test code. The main risk is that adding explicit dependencies to releases.js and form_workflow.js could have introduced problems: because these are used on pages that don't use js bundling at all and rely on script tags instead, all of the dependencies are loaded synchronously at the time the file is loaded, which means that all dependencies' script tags need to have already been included.

Automated test coverage

It's mostly tests. I smoke tested the releases page and the form workflow UI.

QA Plan

No.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@orangejenny orangejenny added the product/invisible Change has no end-user visible impact label Jan 22, 2025
'use strict';
const uiElementKeyValueList = hqImport("hqwebapp/js/ui_elements/bootstrap3/ui-element-key-val-list");

hqDefine("app_manager/js/forms/form_workflow", [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The script tag for this file is here.

ui-element-key-val-list is included in the grandparent template, here, so it'll have already been loaded.

The other dependencies are all widely-used HQ dependencies that are included earlier in hqwebapp/base.html

@@ -1,4 +1,22 @@
hqDefine('app_manager/js/releases/releases', function () {
hqDefine("app_manager/js/releases/releases", [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The script tag for this file is here.

app_manager/js/download_async_modal is directly above it.

app_manager/js/menu is in the parent template here.

The other dependencies are all widely-used HQ dependencies that are included earlier in hqwebapp/base.html

@orangejenny
Copy link
Contributor Author

Linting in #35653

import sinon from "sinon/pkg/sinon";

import Toggles from "hqwebapp/js/toggles";
import FormWorkflow from "app_manager/js/forms/form_workflow";
Copy link
Member

@biyeun biyeun Jan 23, 2025

Choose a reason for hiding this comment

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

is it possible to do an import statement like

import {FormWorkflow} from "app_manager/js/forms/form_workflow";

so that we avoid the FormWorkflow.FormWorkflow situation below?

Copy link
Member

Choose a reason for hiding this comment

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

hmm nvm perhaps that isn't possible because it's not importing from an ESM module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd need to update the return value of app_manager/js/forms/form_workflow and therefore the other code that depends on that module. I might get there - I don't like FormWorkflow. FormWorkflow either - but not in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good to me!

Copy link
Member

@biyeun biyeun left a comment

Choose a reason for hiding this comment

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

This looks good! I just have a minor annoyance with the FormWorkflow.FormWorkflow situation that's happening, but not sure if that's avoidable...

@orangejenny orangejenny merged commit 0968434 into master Jan 23, 2025
12 of 13 checks passed
@orangejenny orangejenny deleted the jls/webpack-app-manager-tests branch January 23, 2025 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants