Skip to content

fix: clear non-exist workspace part in package-lock.json when npm install (fix #5463)#5478

Closed
sun0day wants to merge 4 commits into
npm:latestfrom
sun0day:fix/uselessWorkspace
Closed

fix: clear non-exist workspace part in package-lock.json when npm install (fix #5463)#5478
sun0day wants to merge 4 commits into
npm:latestfrom
sun0day:fix/uselessWorkspace

Conversation

@sun0day
Copy link
Copy Markdown

@sun0day sun0day commented Sep 7, 2022

Clear non-exist workspace part in package-lock.json when npm install

fix #5463

After rm -rf packages/b && npm install , package-lock.json looks like

  • Before PR:
{
 "packages": {
    "": {
      "name": "npm-workspace-remove",
      "version": "1.0.0",
      "license": "ISC",
      "workspaces": [
        "packages/a",
        "packages/b"
      ]
    },
    "node_modules/a": {
      "resolved": "packages/a",
      "link": true
    },
    "packages/a": {
      "version": "1.0.0",
      "license": "ISC"
    },
    "packages/b": {
      "version": "1.0.0",
      "extraneous": true,
      "license": "ISC",
      "devDependencies": {}
    }
  }
}
  • After PR:
{
 "packages": {
    "": {
      "name": "npm-workspace-remove",
      "version": "1.0.0",
      "license": "ISC",
      "workspaces": [
        "packages/a"
      ]
    },
    "node_modules/a": {
      "resolved": "packages/a",
      "link": true
    },
    "packages/a": {
      "version": "1.0.0",
      "license": "ISC"
    }
  }
}

@sun0day sun0day requested a review from a team as a code owner September 7, 2022 13:22
@fritzy
Copy link
Copy Markdown

fritzy commented Sep 14, 2022

@sun0day please add a test that covers this feature

@fritzy fritzy force-pushed the latest branch 2 times, most recently from 3037d35 to f3b0c43 Compare September 14, 2022 23:09
@sun0day
Copy link
Copy Markdown
Author

sun0day commented Sep 16, 2022

@sun0day please add a test that covers this feature

Done.@fritzy

@lukekarrys
Copy link
Copy Markdown
Contributor

@sun0day it looks like this change broke some existing tests. I may attempt to look into this in the coming weeks, but if you can take a look that would help this get merged. ping me if you need any assistance with it.

@lukekarrys lukekarrys added the pr: needs tests requires tests before merging label Sep 21, 2022
@sun0day
Copy link
Copy Markdown
Author

sun0day commented Sep 22, 2022

@sun0day it looks like this change broke some existing tests. I may attempt to look into this in the coming weeks, but if you can take a look that would help this get merged. ping me if you need any assistance with it.

Help needed. I can't figure out why the e workspace node become extraneous: true. Once e is extraneous, this pr addition code will remove it from workspaces. @lukekarrys

image

@lukekarrys lukekarrys self-assigned this Oct 19, 2022
@lukekarrys lukekarrys removed their assignment Nov 12, 2022
@lukekarrys
Copy link
Copy Markdown
Contributor

Apologies for the delay here @sun0day. I'm going to be out for a few weeks so I've unassigned myself in case one of the next CLI Release Managers can unblock this during a future release.

@christopher1986
Copy link
Copy Markdown

christopher1986 commented Jul 8, 2024

It seems like this issue has not yet been resolved in NPM v10.2.4 and this PR is 2 years old.
Is this still a known issue and should the branch conflicts in this PR be resolved in order to fix this problem?

@dandean
Copy link
Copy Markdown

dandean commented Jan 27, 2026

Hello – is this a thing anybody there is maybe interested in merging?

@BenjaminBrienen-CanonPP
Copy link
Copy Markdown

Is this project still maintained?

@owlstronaut
Copy link
Copy Markdown
Contributor

superseded by #9330

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr: needs tests requires tests before merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] package-lock is not cleaned up completely when workspace is removed

8 participants