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

version 8.1.0 seems to be causing a regression #293

Closed
ghiscoding opened this issue Mar 2, 2023 · 5 comments
Closed

version 8.1.0 seems to be causing a regression #293

ghiscoding opened this issue Mar 2, 2023 · 5 comments

Comments

@ghiscoding
Copy link

ghiscoding commented Mar 2, 2023

hello, the latest version 8.1.0 seems to be causing a regression in Lerna-Lite (which is a fork of Lerna that I maintain), I'm not the one who wrote the code (it came from Lerna) with the use of cosmiconfig but anyway the latest version is causing some units tests to fail and I noticed it via Renovate which created a PR with the new versions that runs once a week.

The code can be found on this line in Lerna-Lite

const explorer = cosmiconfigSync('lerna', {
      searchPlaces: ['lerna.json', 'package.json'],
      transform(obj) {
        // cosmiconfig returns null when nothing is found
        if (!obj) {
          return {
            // No need to distinguish between missing and empty,
            // saves a lot of noisy guards elsewhere
            config: {},
            configNotFound: true,
            // path.resolve(".", ...) starts from process.cwd()
            filepath: path.resolve(cwd || '.', 'lerna.json'),
          };
        }

        obj.config = applyExtends(obj.config, path.dirname(obj.filepath));

        return obj;
      },
});

here's an example of a failing unit test (here's the link to the failing PR in Lerna-Lite and below is details of a failing test, there are a few tests failing)

Project › .config › errors when lerna.json is not valid JSON
    expect(received).toThrow(expected)
    Expected asymmetric matcher: ObjectContaining {"name": "ValidationError", "prefix": "JSONError"}
    Received name:    "JSONError"
    Received message: "JSON Error in /tmp/58747792d151a295b7cef1edc3327e65/package.json:··
      3 |   \"devDependencies\": {
      4 |     \"lerna\": \"500.0.0\",
    > 5 |   }
        |   ^
      6 | }
      7 |·
    Unexpected token \"}\" (0x7D) in JSON at position 69 while parsing near \"...erna\\\": \\\"500.0.0\\\",\\n  }\\n}\\n\"··
      3 |   \"devDependencies\": {
      4 |     \"lerna\": \"500.0.0\",
    > 5 |   }
        |   ^
      6 | }
      7 |
    "
          33 |    */
          34 |   constructor(cwd?: string) {
        > 35 |     const explorer = cosmiconfigSync('lerna', {
             |                      ^
          36 |       searchPlaces: ['lerna.json', 'package.json'],
          37 |       transform(obj) {
          38 |         // cosmiconfig returns null when nothing is found

I also saw that another issue #291 was opened for the same version, the error was different but nonetheless a regression for that user as well.

@d-fischer
Copy link
Member

I can't look into this more exactly yet (probably not for a few days) but this is probably, like the other issue, an incompatibility caused by some other package update.

I have made no changes in dependency ranges not do I have any say about the error types of the JSON loader.

To be more future proof, I would advise you to use .toThrow() without arguments.

@d-fischer
Copy link
Member

d-fischer commented Mar 2, 2023

To be clear, #291 could be reproduced with cosmiconfig all the way down to 6.0.0 (possibly even further, I didn't test further because it had an interface change around there, but the point is made). It was not a regression in cosmiconfig, but a breaking change in another package that apparently doesn't follow semver. I suspect the same thing to be happening here.

@d-fischer
Copy link
Member

d-fischer commented Mar 17, 2023

@ghiscoding Could this be the same as #296 (fixed now)?

(I assume it's unlikely since you don't seem to be using a custom loader, but still)

@d-fischer
Copy link
Member

d-fischer commented Mar 17, 2023

Ah, I checked out your project, looked at some of the code, and I get what's happening now.

You're redecorating the JSONError class as ValidationError. This means you can check for your own error class structure in the test.

Now, the cosmiconfig() and cosmiconfigSync() functions may throw when your package.json is invalid, as there are some checks in the initialization.

While, I guess, this is technically breaking, it only happens with an invalid package.json, which, arguably, means the user would have completely different problems, and thus is kind of an obscure error to test for.

Anyway, I'll be sending a PR your way to fix it. This is not a cosmiconfig problem.

@d-fischer d-fischer closed this as not planned Won't fix, can't repro, duplicate, stale Mar 17, 2023
@ghiscoding
Copy link
Author

oh that would be great if you could indeed create a PR on the repo, for now I'm using a fixed version of v8.0.0. Also since I'm not the one who added this piece of code I'm a little bit confused on what needs to change, so definitely a PR would be awesome, thanks a lot. Cheers

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

No branches or pull requests

2 participants