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

fix(ui): Replace deprecated overgear/yup-ast package with demvsystems/yup-ast #390

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

deadlycoconuts
Copy link
Contributor

@deadlycoconuts deadlycoconuts commented Aug 2, 2024

Context

It seems like after upgrading yup from 0.29.1 to 1.4.0, we have broken the its compatibility with the @overgear/yup-ast package. This package is ancient; it's last published 6 years ago and no longer receives any updates because the original contributors have left the company and do not have any permissions/licenses to continue contributing to them. Their replacement is the @demvsystems/yup-ast package, which works almost exactly the same as before.

This package is used when parsing the yup-like schema that's served over an API server endpoint (see https://github.com/caraml-dev/xp/blob/cb06edaaf409077b5abf7fed0b803984b6decf7e/plugins/turing/manager/experiment_manager.go#L38C1-L38C36). We only use this package in one place, when parsing the yup-like schema off the experiment engines endpoint on the Turing API server (this in turn gets its value from the XP experiment manager plugin, which explains why the file quoted above is found in the XP repository and not in Turing). This parsed schema is then used to validate the XP experiment engine config together with the other yup schemas of the Turing router config.

More concretely, this broken package is currently causing this problem whereby the yup validation that gets triggered when a user clicks on the 'Next' page of an accordion form, to end up permanently stuck:
image-20240801-142115

A separate PR in XP has also been created to fix a related bug in the yup-like schema quoted above: caraml-dev/xp#81

Copy link
Contributor

@tiopramayudi tiopramayudi 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 @deadlycoconuts

@deadlycoconuts
Copy link
Contributor Author

Thanks a lot for the quick review @tiopramayudi ! Merging this now!

@deadlycoconuts deadlycoconuts merged commit c1924b4 into caraml-dev:main Aug 5, 2024
12 checks passed
@deadlycoconuts deadlycoconuts deleted the replace_yup_ast branch August 8, 2024 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants