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: clear non-exist workspace part in package-lock.json when npm install (fix #5463) #5478

Open
wants to merge 4 commits into
base: latest
Choose a base branch
from

Conversation

sun0day
Copy link

@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
Contributor

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
Author

sun0day commented Sep 16, 2022

@sun0day please add a test that covers this feature

Done.@fritzy

@lukekarrys
Copy link
Member

@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
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
Copy link
Member

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.

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
3 participants