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

fix(bootstrap): preserve indentation style in package-lock.json when running bootstrap #2955

Merged
merged 1 commit into from Jun 17, 2022

Conversation

richardkazuomiller
Copy link
Contributor

@richardkazuomiller richardkazuomiller commented Jul 28, 2021

Description

This PR fixes preservation of the indentation style used in package.json files by copying package.json files to their backup location, instead of renaming them. This works because write-pkg will use the same indentation style as the package.json file that it is about to overwrite.

Motivation and Context

Running lerna bootstrap always converts spaces to tabs in package-lock.json indentations. This happens because Lerna temporarily replaces package.json with a tab-indented file before running npm install. It would be better if Lerna preserved the indentation style, like npm and write-pkg do.

Fixes #2845

How Has This Been Tested?

I ran npm link with my local copy and tested it in my project by running npm link lerna and lerna bootstrap. My package-lock.json files were saved with space indentations, as expected.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@richardkazuomiller
Copy link
Contributor Author

Hi @evocateur, would it be possible to get a review for this? Not sure who I should ping but I'm mentioning you because you're the most recent contributor 🙏

@omonk
Copy link

omonk commented Aug 5, 2021

Would love to get this reviewed and hopefully merged, thanks for your effort on this one @richardkazuomiller 🤞 it can get merged soon!

@omonk
Copy link

omonk commented Sep 16, 2021

Hi @evocateur - This is still an issue, is there any chance this can be looked at? Thanks!

@yahiarefaiea
Copy link

Hi @evocateur - This is still an issue, is there any chance this can be looked at? Thanks!

Still an issue ++

Running on:
Windows 10
node@14.17.0
npm@6.14.13
lerna@4.0.0

@reediculous456
Copy link

This is still an issue on MacOS 12.1 with node@16.13.2, npm@8.1.0, and lerna@4.0.0

@JamesHenry
Copy link
Member

Hi Folks 👋

I had a look at this just now and I am unfortunately struggling to reproduce the issue with the latest lerna and LTS version of Node, NPM 8.

❯ npx lerna info
lerna notice cli v5.1.4

 Environment info:

  System:
    OS: macOS 12.4
    CPU: (10) arm64 Apple M1 Max
  Binaries:
    Node: 16.15.0 - ~/.volta/tools/image/node/16.15.0/bin/node
    Yarn: 1.22.17 - ~/.volta/tools/image/yarn/1.22.17/bin/yarn
    npm: 8.10.0 - ~/.volta/tools/image/npm/8.10.0/bin/npm
  Utilities:
    Git: 2.32.1 - /usr/bin/git
  npmPackages:
    lerna: ^5.1.4 => 5.1.4 

I'm sure I must be missing something so please can somebody help out by heading over to https://github.com/lerna/repro and creating a minimal reproduction?

As soon as I can confirm the issue we can look to merge this PR.

Many thanks! 🙏

@reediculous456
Copy link

reediculous456 commented Jun 17, 2022

Hey @JamesHenry, here is a simple reproduction of the issue: https://github.com/reediculous456/Lerna-Indentation-Test

  1. Clone the repo
  2. npm install && npm run bootstrap
  3. See the git diff for packages/test/package-lock.json
❯ npx lerna info
lerna notice cli v5.1.4

 Environment info:

  System:
    OS: macOS 12.4
    CPU: (12) x64 Intel(R) Core(TM) i7-8700B CPU @ 3.20GHz
  Binaries:
    Node: 16.15.1 - ~/.nvm/versions/node/v16.15.1/bin/node
    npm: 8.11.0 - ~/.nvm/versions/node/v16.15.1/bin/npm
  Utilities:
    Git: 2.32.0 - /usr/bin/git
  npmPackages:
    lerna: ^5.1.4 => 5.1.4
How I made this example
  1. npm init
  2. npm install -D lerna
  3. npx lerna init
  4. npx lerna create test
  5. npx lerna add lodash packages/test
  6. Issue can now be reproduced when running bootstrap for the first time, or by deleting packages/test/node_modules first

@JamesHenry
Copy link
Member

Thanks so much @reediculous456! It was the third party package in the mix I was missing for the repro I think... but regardless I can reproduce the issue, and with a local build using this PR the issue is no longer present. Let's merge! 🚀

Copy link
Member

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for taking the initiative and putting together this PR @richardkazuomiller!

@JamesHenry JamesHenry merged commit 04cfa52 into lerna:main Jun 17, 2022
@omonk
Copy link

omonk commented Jun 17, 2022

Oh my!! Thank you everyone 🥰

@richardkazuomiller
Copy link
Contributor Author

Oh cool, after almost a year I almost forgot about this 🤣. Thank you!

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 this pull request may close these issues.

Bootstrap reformats package-lock.json to use tabs instead of spaces
5 participants