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

Throws if either dependencies or devDependencies is not defined in package.json #547

Closed
mcous opened this issue Sep 7, 2021 · 3 comments · Fixed by #667
Closed

Throws if either dependencies or devDependencies is not defined in package.json #547

mcous opened this issue Sep 7, 2021 · 3 comments · Fixed by #667

Comments

@mcous
Copy link
Contributor

mcous commented Sep 7, 2021

Bug report

#527 introduced functionality to check for the presence of PostCSS in a project's package.json in order to log a warning if the version didn't match. The functionality is implemented by parsing package.json and checking pkg.dependencies.postcss and pkg.devDependencies.postcss.

However, neither dependencies nor devDependencies is a required field of package.json. If either of these fields are missing in package.json, a TypeError: Cannot read property 'postcss' of undefined will be raised.

// valid package.json that will cause a TypeError
{
    "name": "my-project-without-dependencies",
    "version": "0.0.0",
    "devDependencies": {
        // ...
    }
}

// workaround - add an empty dependencies object
{
    "name": "my-project-without-dependencies",
    "version": "0.0.0",
    "dependencies": {},
    "devDependencies": {
        // ...
    }
}

Actual Behavior

See above

Expected Behavior

A project need not define dependencies nor devDependencies for this loader to work. In my specific use case, the project in question is part of a monorepo, where all dev dependencies are hoisted to the workplace package.json and the project's package.json has no dev dependencies. I'm not sure if having postcss declared in a monorepo root package.json but not a sub-project package.json would cause further issues.

As an aside (and after reading through the initial issue thread at #507), I understand why this functionality was implemented, but in my personal opinion, "checking that peer dependencies are properly installed" should be outside the scope of any functionality in this module besides declaring the peer dep in package.json.

How Do We Reproduce?

  1. Initialize a project with postcss-loader and webpack
  2. Delete dependencies in package.json
  3. Run webpack
  4. Observe TypeError raise

Please paste the results of npx webpack info here, and mention other relevant information

❯ npx webpack info     

  System:
    OS: macOS 10.15.7
    CPU: (20) x64 Intel(R) Xeon(R) W-2150B CPU @ 3.00GHz
    Memory: 8.64 GB / 64.00 GB
  Binaries:
    Node: 12.20.0 - ~/.nvs/default/bin/node
    Yarn: 1.22.10 - ~/.nvs/default/bin/yarn
    npm: 6.14.8 - ~/.nvs/default/bin/npm
  Browsers:
    Chrome: 93.0.4577.63
    Firefox: 91.0.2
    Firefox Developer Edition: 84.0
    Safari: 14.1.2
  Monorepos:
    Yarn Workspaces: 1.22.10
    Lerna: 3.22.1
  Packages:
    favicons-webpack-plugin: ^5.0.2 => 5.0.2 
    html-webpack-plugin: ^5.3.2 => 5.3.2 
    optimize-css-assets-webpack-plugin: ^6.0.1 => 6.0.1 
    script-ext-html-webpack-plugin: ^2.1.4 => 2.1.5 
    terser-webpack-plugin: ^5.2.3 => 5.2.3 
    webpack: ^5.52.0 => 5.52.0 
    webpack-bundle-analyzer: ^4.4.2 => 4.4.2 
    webpack-cli: ^4.8.0 => 4.8.0 
    webpack-dev-server: ^4.1.1 => 4.1.1 
    webpack-merge: ^5.8.0 => 5.8.0 
    webpack-node-externals: ^3.0.0 => 3.0.0
@alexander-akait
Copy link
Member

alexander-akait commented Sep 7, 2021

You need always install postcss to work with this loader due https://github.com/webpack-contrib/postcss-loader/blob/master/package.json#L40, but yes, checking dependencies should be out of scope module

sorry - closed by mistake

@alexander-akait
Copy link
Member

To be honest, I had the same opinion as you, but @ai as main developer of postcss wanted to do it, but I agree it is wrong and invalid

@alexander-akait
Copy link
Member

alexander-akait commented Sep 7, 2021

npm v7 automatically install peer deps, yarn throws an errors (for me it is right behavior, but this question about another problem), npm v6 output warnings, developer should read output from package manager, and packages should not check dependencies except optionalDependencies (try/catch)

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.

2 participants