avajs / ava

Node.js test runner that lets you develop with confidence 🚀
MIT License
20.74k stars 1.41k forks source link

`readPackageSync()` could be simplified, 17 fewer deps #2853

Closed live627 closed 3 years ago

live627 commented 3 years ago

read-pkg is designated to read package files and optionally normalize them using normalize-package-data. Thing is, only the type attribute is used. https://github.com/avajs/ava/blob/b0b76a034f106dfa34f4ca63da7a3700717857d0/lib/cli.js#L314-L323 Doesn't need to be normalized because it is validated https://github.com/avajs/ava/blob/b0b76a034f106dfa34f4ca63da7a3700717857d0/lib/module-types.js#L1-L25

Could this be simplified?

    let pkg;
    try {
-       pkg = readPackageSync({cwd: projectDir});
+       pkg = parseJson(fs.readFileSync(path.resolve(projectDir, 'package.json'), 'utf8'));
    } catch (error) {
        if (error.code !== 'ENOENT') {
            throw error;

read-pkg could then be dropped

ava@4.0.0-alpha.2 (202 deps)
│   ...
├─┬ read-pkg@5.2.0 (32 deps, 554.2kb, 281 files)
│ ├── @types/normalize-package-data@2.4.1 (6.14kb, 4 files)
│ ├─┬ normalize-package-data@2.5.0 (13 deps, 350.19kb, 173 files)
│ │ ├── hosted-git-info@2.8.9 (25.21kb, 7 files)
│ │ ├─┬ resolve@1.20.0 (4 deps, 167.67kb, 122 files)
│ │ │ ├─┬ is-core-module@2.7.0 (2 deps, 51.03kb, 27 files)
│ │ │ │ ╰─┬ has@1.0.3 (1 dep, 27.27kb, 17 files)
│ │ │ │   ╰── function-bind@1.1.1 (24.56kb, 12 files)
│ │ │ ╰── path-parse@1.0.7 (4.41kb, 4 files)
│ │ ├── semver@5.7.1 (60.13kb, 7 files)
│ │ ╰─┬ validate-npm-package-license@3.0.4 (5 deps, 71.15kb, 26 files)
│ │   ├─┬ spdx-correct@3.1.1 (4 deps, 54.94kb, 22 files)
│ │   │ ├── spdx-expression-parse@3.0.1 (🔗, 2 deps, 23.62kb, 14 files)
│ │   │ ╰── spdx-license-ids@3.0.10 (9.45kb, 4 files)
│ │   ╰─┬ spdx-expression-parse@3.0.1 (2 deps, 23.62kb, 14 files)
│ │     ├── spdx-exceptions@2.3.0 (2.6kb, 3 files)
│ │     ╰── spdx-license-ids@3.0.10 (9.45kb, 4 files)
│ ├─┬ parse-json@5.2.0 (15 deps, 163.03kb, 85 files)
│ │ ├─┬ @babel/code-frame@7.14.5 (10 deps, 127.88kb, 58 files)
│ │ │ ╰─┬ @babel/highlight@7.14.5 (9 deps, 121.09kb, 54 files)
│ │ │   ├── @babel/helper-validator-identifier@7.15.7 (18.55kb, 7 files)
│ │ │   ├─┬ chalk@2.4.2 (6 deps, 83.07kb, 38 files)
│ │ │   │ ├─┬ ansi-styles@3.2.1 (2 deps, 44.62kb, 18 files)
│ │ │   │ │ ╰─┬ color-convert@1.9.3 (1 dep, 35.47kb, 14 files)
│ │ │   │ │   ╰── color-name@1.1.3 (9.14kb, 7 files)
│ │ │   │ ├── escape-string-regexp@1.0.5 (🔗, 2.63kb, 4 files)
│ │ │   │ ╰─┬ supports-color@5.5.0 (1 dep, 9.53kb, 9 files)
│ │ │   │   ╰── has-flag@3.0.0 (3.05kb, 4 files)
│ │ │   ╰── js-tokens@4.0.0 (14.72kb, 5 files)
│ │ ├─┬ error-ex@1.3.2 (1 dep, 12.78kb, 12 files)
│ │ │ ╰── is-arrayish@0.2.1 (3.96kb, 8 files)
│ │ ├── json-parse-even-better-errors@2.3.1 (10.18kb, 5 files)
│ │ ╰── lines-and-columns@1.1.6 (6.91kb, 6 files)
│ ╰── type-fest@0.6.0 (28.93kb, 14 files)

and replaced with parse-json

ava@4.0.0-alpha.2 (188 deps)
│   ...
├─┬ parse-json@5.2.0 (15 deps, 163.03kb, 85 files)
│ ├─┬ @babel/code-frame@7.14.5 (10 deps, 127.88kb, 58 files)
│ │ ╰─┬ @babel/highlight@7.14.5 (9 deps, 121.09kb, 54 files)
│ │   ├── @babel/helper-validator-identifier@7.15.7 (18.55kb, 7 files)
│ │   ├─┬ chalk@2.4.2 (6 deps, 83.07kb, 38 files)
│ │   │ ├─┬ ansi-styles@3.2.1 (2 deps, 44.62kb, 18 files)
│ │   │ │ ╰─┬ color-convert@1.9.3 (1 dep, 35.47kb, 14 files)
│ │   │ │   ╰── color-name@1.1.3 (9.14kb, 7 files)
│ │   │ ├── escape-string-regexp@1.0.5 (2.63kb, 4 files)
│ │   │ ╰─┬ supports-color@5.5.0 (1 dep, 9.53kb, 9 files)
│ │   │   ╰── has-flag@3.0.0 (3.05kb, 4 files)
│ │   ╰── js-tokens@4.0.0 (14.72kb, 5 files)
│ ├─┬ error-ex@1.3.2 (1 dep, 12.78kb, 12 files)
│ │ ╰── is-arrayish@0.2.1 (3.96kb, 8 files)
│ ├── json-parse-even-better-errors@2.3.1 (10.18kb, 5 files)
│ ╰── lines-and-columns@1.1.6 (6.91kb, 6 files)

That in turn could also be dropped in favor of JSON.parse, but then the enhanced error messages would be lost.

novemberborn commented 3 years ago

I don't think we need enhanced error messages here, if the package.json is corrupted all kinds of other things would also fail. fs and JSON should suffice.

alessandroasm commented 3 years ago

I would like to work on that, if no one is assigned to it :)

live627 commented 3 years ago

Neat, now I don't have to 😃

On Mon, Oct 4, 2021 at 5:32 PM Alessandro Menezes @.***> wrote:

I would like to work on that, if no one is assigned to it :)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/avajs/ava/issues/2853#issuecomment-933961840, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADJNN3SCURCNFJTQR3I2F3UFJBQVANCNFSM5FEVG2SQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

alessandroasm commented 3 years ago

Hi. I've opened a draft PR for this (#2863). I tried my best to do a minimal change to the package-lock.json file, but npm really wants to add dev: true to a bunch of dependencies.

Well, if you have any suggestions, please let me know.

alessandroasm commented 3 years ago

@novemberborn @live627 Hello, the PR is ready for a few days now. Is there anything else I should do to get it approved & merged?

Please let me know. Thanks in advance.

novemberborn commented 3 years ago

@alessandroasm I haven't had a chance to look at it yet.