depfu / feedback

🤔 Question, bugs and feedback for Depfu
https://depfu.com
MIT License
8 stars 4 forks source link

Respect indentation in `package-lock.json` #72

Closed AlexWayfer closed 8 months ago

AlexWayfer commented 10 months ago

Hello.

I have a project with NPM and lock-file in git.

Depfu sends me PRs with group update, including lock-file changes: https://github.com/AlexWayfer/alexwayfer.name/pull/229

But it replaces all tabs in package-lock.json file from my local machine to spaces: https://github.com/AlexWayfer/alexwayfer.name/pull/201/files

Then I pull changes, NPM scripts don't work (node_modules/ not updated locally), I'm running npm install and getting reverse update: https://github.com/AlexWayfer/alexwayfer.name/commit/8f19e407077282af7bb9a76c7551efc0ca885a6c

It seems like you've hardcoded 2 spaces indentation, but my NPM has tabs for some reason and I see nothing wrong with it. It's also displayed in the project's .editorconfig, but I think Depfu should just respect current file indentation without external checks.

halfbyte commented 9 months ago

Hey @AlexWayfer thanks for your feedback. npm does indeed normally preserve indentation and I've found the place where we mess this up, so expect a fix relatively soon.

I want to mention, though, that you have to coerce npm into using tabs by running the lockfile through a linter that replaces spaces with tabs. I acknowledge that we should not interfere with that, but also, I would normally treat package-lock.json almost as a binary file. Most lockfiles are too big to be readable, the diffs are often not displayed by GitHub etc. etc. so I would probably always exclude the lockfile from linting. There are many different ways how a linter or formatter could format JSON files and while npm (And hopefully soon Depfu) respects indentation it is not the only thing that could produce odd diffs.

halfbyte commented 9 months ago

@AlexWayfer Can you please check if this problem still exists? I just deployed a potential fix.

AlexWayfer commented 8 months ago

@AlexWayfer Can you please check if this problem still exists? I just deployed a potential fix.

I can't merge it right now due to outdated stylelint-no-unsupported-browser-features, but the PR seems clean: https://github.com/AlexWayfer/alexwayfer.name/pull/239/files

Thank you.