search

google / zx

A tool for writing better scripts
https://google.github.io/zx/
Apache License 2.0
42.8k stars 1.09k forks source link

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

Closed szabopeterakos closed 1 week ago

szabopeterakos commented 1 month ago

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

szabopeterakos commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/google/zx/commit/d7d074d5945b33d85c8e68085839d16b2c55b943 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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.

corporateuser commented 2 weeks ago

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

antonmedv commented 2 weeks ago

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

antongolub commented 2 weeks ago

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