[go: up one dir, main page]

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

Since 8.1.4 YAML.parseAllDocuments is not allowed from zx #870

Closed
szabopeterakos opened this issue Jul 17, 2024 · 9 comments · Fixed by #879
Closed

Since 8.1.4 YAML.parseAllDocuments is not allowed from zx #870

szabopeterakos opened this issue Jul 17, 2024 · 9 comments · Fixed by #879

Comments

@szabopeterakos
Copy link

Expected Behavior

Actual Behavior

TypeError: YAML.parseAllDocuments is not a function

Steps to Reproduce

  1. update zx to 8.1.4
  2. try to use YAML.parseAllDocuments(x)

Specifications

  • zx version: 8.1.4
  • Platform: macos
  • Runtime: node
@szabopeterakos
Copy link
Author
szabopeterakos commented Jul 17, 2024

Before this release YAML.parseAllDocuments was possible, after release 8.1.4 its no longer there. It should be another way to use it since now, or is it an accidental removal?

@antongolub
Copy link
Collaborator
antongolub commented Jul 17, 2024

I'm afraid, the mentioned extra API was previously accidentally added. zx is aimed to provide just a basic YAML helpers symmetric with JSON. This will help us change the implementation in the future if needed, avoiding breaking changes.

@corporateuser
Copy link
corporateuser commented Jul 19, 2024

I'm afraid, the mentioned extra API was previously accidentally added. zx is aimed to provide just a basic YAML helpers symmetric with JSON. This will help us change the implementation in the future if needed, avoiding breaking changes.

Do not agree with your statement:
From zx documentation yaml zx promising the whole YAML package and now with d7d074d you've decided to remove APIs which you for some reason find redundant.
This is breaking change, which should not be done in a minor update for sure.

@nishnp
Copy link
nishnp commented Jul 19, 2024

I'm afraid, the mentioned extra API was previously accidentally added. zx is aimed to provide just a basic YAML helpers symmetric with JSON. This will help us change the implementation in the future if needed, avoiding breaking changes.

Do not agree with your statement: From zx documentation yaml zx promising the whole YAML package and now with d7d074d you've decided to remove APIs which you for some reason find redundant. This is breaking change, which should not be done in a minor update for sure.

I agree... even If it has to be removed, should not be done in a minor update.

@antongolub
Copy link
Collaborator
antongolub commented Jul 19, 2024

The problem is that we haven't covered this part of the API with tests. That's why it failed so badly. We will definitely invest time to increase vendor chunk test coverage.

you've decided to remove APIs which you for some reason find redundant.

This is how it happened:
— We need a YAML parser on board.
js-yaml?
yaml.load, yaml.dump... Probably it would be better to remap like in JSON?
yaml already has the necessary API and is updated more frequently.
— Great, then let's take it.

@corporateuser
Copy link

I see your point, but initial description of zx goods does not describe that there is a limited set of yaml package, it describes that the whole yaml package available. So if it is really needed to remove additional methods it is like removing the whole package, which breaks backward compatibility. I do not see any point in breaking compatibility within the same major version and do not see any point in still having yaml package and dependencies and artificially removing its methods.

billimek added a commit to billimek/k8s-gitops that referenced this issue Jul 23, 2024
Signed-off-by: billimek <jeff@billimek.com>
@corporateuser
Copy link

So, there is no plan to revert the mentioned change, correct?

@antonmedv
Copy link
Collaborator

Can we create a small stub for this methods? @antongolub not copy original defs, but createca simple method which returns any?

@antongolub
Copy link
Collaborator

I don't mind, but let's also mark it as deprecated.

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 a pull request may close this issue.

5 participants