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

UIE-204 Narrow Ajax Usage pt24 (final) #5240

Merged
merged 3 commits into from
Jan 31, 2025

Conversation

msilva-broad
Copy link
Contributor

  • narrow Ajax() usage within last of src/libs/ajax area modules to call Ajax().SubAreaX directly.
  • demoted Ajax() to no longer be exported from ajax.ts. It is now called AjaxTestingRoot and only a setupAjaxTestUtil method is exposed.
  • setupAjaxTestUtil is called from appLoader.js since it is no longer incedentally initalized through ajax.ts module load.
  • unit tests added for setupAjaxTestUtil.
  • improve mock types where possible

Jira Ticket: https://broadworkbench.atlassian.net/browse/[Ticket #]

Summary of changes:

What

  • Ajax() super-object has been an anti-pattern of sorts, that has been pervasive and problematic in our code base. It's caused many lost hours to circular dependency trouble-shooting, and overly-entwined dependency footprint in general. Previous investments carefully unraveled the circular dependencies, but with Ajax() super-object still kicking around, they could easily be re-introduced.
  • The Ajax() super-object facilitated data-call unification and bespoke data-call override mechanics that facilitated script-like test setup convenience for some of our end2end tests to perform workspace, runitme, etc preconditions through direct data calls before the main test scenario. This seemed more valuable when end2end tests were the only real automated testing footprint for Terra UI. But we have now invested in a growing set of unit tests, which provide great coverage, speed, and other benefits over end2end tests. So we have been reducing the number and complexity of end2end tests, making the Ajax() testing root pattern something we should probably discourage. It is kept for current compatibility, but would be nice to one day simplify out it's need in the remaining end2end tests that still use it.
  • This series of PRs has also taken the opportunity to optimize type safety in many of our unit tests. Lack of type safety on mock signatures is a nasty gap, since it would leave us blind to contract drift. Tests would continue to pass even though their mocks no longer reflect reality. This has been addressed in all tests that were mocking Ajax() subareas.
  • Ajax mocking was also clunky since tests has to mock the outer and inner function/contract layers. This has also been cleaned up.

Why

  • improve code maintainability, improve modularity and potential code sharing by simplifying dependency graph.

Testing strategy

@msilva-broad msilva-broad changed the title UIE-204 Narrow Ajax Usage pt24 UIE-204 Narrow Ajax Usage pt24 (final) Jan 31, 2025
@msilva-broad msilva-broad force-pushed the msilva/UIE-204_Narrow-ajax-usage_pt24 branch from 7904c0c to 883b5ed Compare January 31, 2025 16:36
@msilva-broad msilva-broad marked this pull request as ready for review January 31, 2025 16:45
@msilva-broad msilva-broad requested review from a team as code owners January 31, 2025 16:45
Copy link
Contributor

@kevinmarete kevinmarete left a comment

Choose a reason for hiding this comment

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

Some minor comments but looks good to me!

Comment on lines 17 to 18
import { WorkspaceData } from 'src/libs/ajax/WorkspaceDataService';
import { Capabilities, Capability } from 'src/libs/ajax/WorkspaceDataService';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { WorkspaceData } from 'src/libs/ajax/WorkspaceDataService';
import { Capabilities, Capability } from 'src/libs/ajax/WorkspaceDataService';
import { Capabilities, Capability, WorkspaceData } from 'src/libs/ajax/WorkspaceDataService';

You are importing WorkspaceDataService multiple times, recommend just adding WorkspaceData to the existing import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 2 to 3
import { GoogleStorage } from 'src/libs/ajax/GoogleStorage';
import { GCSItem } from 'src/libs/ajax/GoogleStorage';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { GoogleStorage } from 'src/libs/ajax/GoogleStorage';
import { GCSItem } from 'src/libs/ajax/GoogleStorage';
import { GCSItem, GoogleStorage } from 'src/libs/ajax/GoogleStorage';

Multiple imports, consider importing once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

- narrow Ajax() usage within last of src/libs/ajax area modules to call Ajax().SubAreaX directly.
- demoted Ajax() to no longer be exported from ajax.ts.  It is now called AjaxTestingRoot and only a setupAjaxTestUtil method is exposed.
- setupAjaxTestUtil is called from appLoader.js since it is no longer incedentally initalized through ajax.ts module load.
- unit tests added for setupAjaxTestUtil.
- improve mock types where possible
- fixed lingering ajax mock reference
- fixed duplicated imports
@msilva-broad msilva-broad force-pushed the msilva/UIE-204_Narrow-ajax-usage_pt24 branch from b2f11ed to 1e65d98 Compare January 31, 2025 19:44
Copy link
Collaborator

@LizBaldo LizBaldo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for doing this!

@msilva-broad msilva-broad added this pull request to the merge queue Jan 31, 2025
Merged via the queue into dev with commit 2b3c543 Jan 31, 2025
11 checks passed
@msilva-broad msilva-broad deleted the msilva/UIE-204_Narrow-ajax-usage_pt24 branch January 31, 2025 20:52
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.

4 participants