Skip to content

Conversation

@usavkov-epam
Copy link
Contributor

@usavkov-epam usavkov-epam commented Apr 17, 2025

JIRA https://folio-org.atlassian.net/browse/STTYPES-21

Changes in BE (#1108, #1104):

  • userLimit type changed from number to string.
  • Deprecated fields removed (e.g., alerts, reportingCodes).

@usavkov-epam usavkov-epam self-assigned this Apr 17, 2025
@usavkov-epam usavkov-epam requested a review from a team as a code owner April 17, 2025 08:05
@usavkov-epam usavkov-epam requested a review from a team April 17, 2025 08:05
@github-actions
Copy link

Jest Unit Test Results

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 474b55a. ± Comparison against base commit 0e9c064.

@sonarqubecloud
Copy link

@zburke zburke requested a review from ncovercash April 17, 2025 11:52
Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

@usavkov-epam , Uh, I know I'm late to the party and what's really happening here is reflecting backend changes into the response-shapes we have to deal with on the frontend, but can you explain why userLimit's type is changing from number to string?

The Trillium release of Stripes is not intended to be breaking, but bumping stripes-types to v4 works against that, so this change puts us in an awkward position. @ncovercash , @JohnC-80 , a penny for your thoughts on how to handle type-changes like this one (if we agree it is legit) or other things (e.g. type-narrowing). What is conventional in TypeScript-land?

@zburke
Copy link
Member

zburke commented Apr 17, 2025

@ncovercash , @JohnC-80 , strictly from the point of view of compatibility, is there any merit (or danger) to defining userLimit like

userLimit?: string | number;

instead of just number to avoid the breaking change? Related Slack converation.

@usavkov-epam
Copy link
Contributor Author

usavkov-epam commented Apr 18, 2025

@zburke I believe you’ve already seen Dennis Bridges’ message in Slack (link), where he explains the business logic behind changing the field type—specifically, that some vendors may need to include alphanumeric characters rather than just numbers.

I agree that, at first glance, simply changing the field type seems like a minor adjustment, but it has led to a significant number of changes across the project, which does raise some concerns.

Regarding the userLimit?: string | number; approach—to be honest, this doesn’t seem like the right solution either. It feels more like an attempt to make TypeScript compile the code without introducing a breaking change, rather than a proper fix. In reality, the backend won’t support the number type for this field (and the same applies to string if an older version of the order-lines interface is used). Even if TypeScript doesn’t throw an error when compiling the UI for a numeric value, sending such a value to the backend would still result in an error.

@zburke
Copy link
Member

zburke commented Apr 18, 2025

Thanks for weighing in, @usavkov-epam. In yet another Slack conversation, we concluded that ACQ types really shouldn't be part of stripes-types in the first place and that instead of implementing a breaking change here and now, we should instead:

  1. leave the ACQ types here alone
  2. remove the ACQ types here in a future breaking release
  3. add correct ACQ types to 🥁 ACQ, which is the only the place they should have been added to begin with

If you're OK with this plan, let's close this PR. I will open a ticket in STYPES to remove the ACQ types in Umbrella-leaf (assuming that is the next release with breaking changes).

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