Add prettier and eslint #638

theimbender · 2021-02-15T14:23:46Z

Adds prettier and eslint with pre-commit hooks so that relevant files are linted and formatted during commit
Latest version of Encore now supports latest version of sass-loader, so bump all dependencies
Remove the one usage of optional chaining, it wasn't needed and violates the new eslint rules
Remove unused popper.js dependency
Expose script for composer test so you can run the project's tests with the same version of PHPUnit that the CI checks will use

theimbender · 2021-02-15T15:12:47Z

Was also thinking of introducing codsniffer linting, what standards does this project follow? Just PSR2?

theimbender · 2021-02-15T15:14:01Z

Ah, never mind, I just noticed the cs fixer config

alcohol · 2021-02-16T07:29:16Z

Do you reckon there is anything we can do about the following? I know some are indirect dependencies, but some are root dependencies.

npm WARN deprecated urix@0.1.0: Please see https://github.com/lydell/urix#deprecated
npm WARN deprecated har-validator@5.1.5: this library is no longer supported
npm WARN deprecated resolve-url@0.2.1: https://github.com/lydell/resolve-url#deprecated
npm WARN deprecated eslint-loader@4.0.2: This loader has been deprecated. Please use eslint-webpack-plugin
npm WARN deprecated request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142
npm WARN deprecated popper.js@1.16.1: You can find the new Popper v2 at @popperjs/core, this package is dedicated to the legacy v1

stof · 2021-02-16T09:37:07Z

package-lock.json

-        "popper.js": "^1.14.3",
-        "sass": "^1.32.6",
-        "sass-loader": "^10.0.0"
+        "eslint-loader": "^4.0.2",


please don't use the deprecated loader here

I was thinking about that but encore doesn't yet have first class support for eslint-webpack-plugin. Would be better to write a custom rule for that now and remove it later on?

Eh, it's just eventually replacing two lines of code with one hopefully, let me add that so it makes sense

ok, then maybe wait for Encore to support it.

I already pushed a new commit that adds the plugin for that, that should continue to work even after encore supports it natively. Gonna be cleanup either way so might as well use a dependency that isn't deprecated.

Sorry, i was committing too fast

well, as that's simple enough, it is fine with me

stof · 2021-02-16T09:58:48Z

popper.js is not an unused dependency. It satisfies the peerDependency of bootstrap

@alcohol for the warnings, the popper.js one can only be solved once Bootstrap 5.0 is released (and Satis upgrades to use it) as it is the one migrating to the new @popperjs/core
for har-validator and request, this looks like a bug in the way the lock file was updated after switching from node-sass to sass, as they are dependencies of node-sass which is still there in the lock file.
For resolve-url and urix, they are dependencies of rework, which is used optionally by the resolve-url-loader. This is being worked on at bholloway/resolve-url-loader#160 for the upcoming v4 of the loader.

stof · 2021-02-16T10:06:49Z

OK, I understood it. npm 7 is installing peer dependencies by default, even when they are optional peer dependencies (which looks weird to me as there is no way to omit them then)

theimbender · 2021-02-16T10:46:52Z

Maybe that was why popper.js was explicitly in package.json before? Because npm wasn't previously installing that entire dependency chain so it needed to be there or Bootstrap wouldn't work?

stof · 2021-02-16T10:55:04Z

yeah, and that's actually the expected way for peer dependencies to work (neither pnpm nor yarn are automatically installing missing peer dependencies, and npm 6 was not doing it either)

alcohol · 2021-02-16T10:57:22Z

Since I seem unable to push to your branch, I added some changes here: https://github.com/composer/satis/tree/theimbender-dependency-update

Any idea why npm ci fails on npm 6.x but not on 7.x ?

theimbender · 2021-02-16T10:58:45Z

Probably the peer dependency thing, let me add back popper

alcohol · 2021-02-16T10:59:22Z

https://github.com/composer/satis/runs/1909979666?check_suite_focus=true

alcohol · 2021-02-16T10:59:32Z

Seems resolved though now that I updated to node 15.

alcohol · 2021-02-16T11:02:21Z

Also if I run npm install locally from scratch I get quite a different lock file / set of dependencies:

$ npm --version
7.5.3
$ rm -rf node_modules package-lock.json 
$ npm i
npm WARN ERESOLVE overriding peer dependency
npm WARN Found: webpack@5.22.0
npm WARN node_modules/webpack
npm WARN   webpack@"^5.12" from @symfony/webpack-encore@1.1.0
npm WARN   node_modules/@symfony/webpack-encore
npm WARN     dev @symfony/webpack-encore@"^1.1.0" from the root project
npm WARN   7 more (eslint-loader, sass-loader, assets-webpack-plugin, ...)
npm WARN 
npm WARN Could not resolve dependency:
npm WARN peer webpack@"^4.0.0" from friendly-errors-webpack-plugin@2.0.0-beta.2
npm WARN node_modules/friendly-errors-webpack-plugin
npm WARN   friendly-errors-webpack-plugin@"^2.0.0-beta.1" from @symfony/webpack-encore@1.1.0
npm WARN   node_modules/@symfony/webpack-encore
npm WARN deprecated urix@0.1.0: Please see https://github.com/lydell/urix#deprecated
npm WARN deprecated resolve-url@0.2.1: https://github.com/lydell/resolve-url#deprecated
npm WARN deprecated eslint-loader@4.0.2: This loader has been deprecated. Please use eslint-webpack-plugin
npm WARN deprecated popper.js@1.16.1: You can find the new Popper v2 at @popperjs/core, this package is dedicated to the legacy v1

> prepare
> husky install

husky - Git hooks installed

added 1218 packages, and audited 1219 packages in 29s

97 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities

stof · 2021-02-16T11:02:41Z

Any idea why npm ci fails on npm 6.x but not on 7.x ?

that's because your package-lock.json uses the v2 format introduced in npm 7.x

alcohol · 2021-02-16T11:03:28Z

I see. Is there a way to specify what node/npm version our project uses, like composer can reference what php version we support?

theimbender · 2021-02-16T11:04:09Z

Yes, https://docs.npmjs.com/cli/v7/configuring-npm/package-json#engines

theimbender · 2021-02-16T11:04:36Z

Should i require npm 7.x?

alcohol · 2021-02-16T11:04:37Z

Great, any recommendations?

alcohol · 2021-02-16T11:05:30Z

If 7.x introduces changes that are not BC with 6.x, then yes I would prefer to target the latest versions from here on forwards.

stof · 2021-02-16T11:05:31Z

if we use a npm 7.x lock file, we should indeed require npm 7 in the package.json to avoid weird issues.

alcohol · 2021-02-16T11:22:41Z

Ok I rebased https://github.com/composer/satis/tree/theimbender-dependency-update

But if I locally run rm -rf node_modules package-lock.json && npm install I still get quite a different result. Not sure if that is because I am on Linux and you are on Mac, or because the dependencies desynced based on adjustments you made that somehow didn't propagate properly.

theimbender · 2021-02-16T11:26:54Z

ok, let me run that exact same command because there might be something wonky with my node_modules then

theimbender · 2021-02-16T11:31:00Z

yeah i'm gonna just do that every time i commit any change involving a lock file, less headache. The command npm run build didn't generate any different output for me, so just the lock file needed to be fixed. Does this have the same output as your local?

alcohol · 2021-02-16T11:33:07Z

Yep! I just get the one expected diff now:

$ git diff
diff --git i/package-lock.json w/package-lock.json
index 5647063..a5e4f1c 100644
--- i/package-lock.json
+++ w/package-lock.json
@@ -2467,7 +2467,6 @@
       "dependencies": {
         "anymatch": "~3.1.1",
         "braces": "~3.0.2",
-        "fsevents": "~2.3.1",
         "glob-parent": "~5.1.0",
         "is-binary-path": "~2.1.0",
         "is-glob": "~4.0.1",

Nothing else. 👍

alcohol · 2021-02-16T11:34:35Z

Merged via aba12d6...ff2da4f

theimbender force-pushed the dependency-update branch 2 times, most recently from c13bf0d to 8e57fcc Compare February 15, 2021 14:31

theimbender force-pushed the dependency-update branch 3 times, most recently from 9d1d01d to b292c49 Compare February 15, 2021 15:27

Add prettier and eslint

1b5ce0f

theimbender force-pushed the dependency-update branch from b292c49 to 1b5ce0f Compare February 15, 2021 15:29

stof reviewed Feb 16, 2021

View reviewed changes

Replace eslint-loader with eslint-webpack-plugin

06dc1d9

Require npm >= 7.0 to support v2 lock file

83d5c6f

Regenerate lock file

1ef8aef

alcohol closed this Feb 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add prettier and eslint #638

Add prettier and eslint #638

theimbender commented Feb 15, 2021

theimbender commented Feb 15, 2021

theimbender commented Feb 15, 2021

alcohol commented Feb 16, 2021

stof Feb 16, 2021

theimbender Feb 16, 2021

theimbender Feb 16, 2021

stof Feb 16, 2021

theimbender Feb 16, 2021

theimbender Feb 16, 2021

stof Feb 16, 2021

stof commented Feb 16, 2021

stof commented Feb 16, 2021

theimbender commented Feb 16, 2021

stof commented Feb 16, 2021

alcohol commented Feb 16, 2021

theimbender commented Feb 16, 2021

alcohol commented Feb 16, 2021

alcohol commented Feb 16, 2021

alcohol commented Feb 16, 2021

stof commented Feb 16, 2021

alcohol commented Feb 16, 2021 •

edited

Loading

theimbender commented Feb 16, 2021

theimbender commented Feb 16, 2021

alcohol commented Feb 16, 2021

alcohol commented Feb 16, 2021

stof commented Feb 16, 2021

alcohol commented Feb 16, 2021 •

edited

Loading

theimbender commented Feb 16, 2021

theimbender commented Feb 16, 2021

alcohol commented Feb 16, 2021

alcohol commented Feb 16, 2021 •

edited

Loading

Add prettier and eslint #638

Add prettier and eslint #638

Conversation

theimbender commented Feb 15, 2021

theimbender commented Feb 15, 2021

theimbender commented Feb 15, 2021

alcohol commented Feb 16, 2021

stof Feb 16, 2021

Choose a reason for hiding this comment

theimbender Feb 16, 2021

Choose a reason for hiding this comment

theimbender Feb 16, 2021

Choose a reason for hiding this comment

stof Feb 16, 2021

Choose a reason for hiding this comment

theimbender Feb 16, 2021

Choose a reason for hiding this comment

theimbender Feb 16, 2021

Choose a reason for hiding this comment

stof Feb 16, 2021

Choose a reason for hiding this comment

stof commented Feb 16, 2021

stof commented Feb 16, 2021

theimbender commented Feb 16, 2021

stof commented Feb 16, 2021

alcohol commented Feb 16, 2021

theimbender commented Feb 16, 2021

alcohol commented Feb 16, 2021

alcohol commented Feb 16, 2021

alcohol commented Feb 16, 2021

stof commented Feb 16, 2021

alcohol commented Feb 16, 2021 • edited Loading

theimbender commented Feb 16, 2021

theimbender commented Feb 16, 2021

alcohol commented Feb 16, 2021

alcohol commented Feb 16, 2021

stof commented Feb 16, 2021

alcohol commented Feb 16, 2021 • edited Loading

theimbender commented Feb 16, 2021

theimbender commented Feb 16, 2021

alcohol commented Feb 16, 2021

alcohol commented Feb 16, 2021 • edited Loading

alcohol commented Feb 16, 2021 •

edited

Loading

alcohol commented Feb 16, 2021 •

edited

Loading

alcohol commented Feb 16, 2021 •

edited

Loading