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: packages in the workspace not being under node_modules #8576

Merged
merged 12 commits into from
Oct 10, 2024

Conversation

beyondkmp
Copy link
Collaborator

@beyondkmp beyondkmp commented Oct 9, 2024

fix develar/app-builder#141

reproduced error info

  ● yarn several workspaces

    /tmp/et-db411cd21f3bbd882f3a5a30c58db6ff/t-kIKFAh/test-project-2/packages/foo/package.json must be under /tmp/et-db411cd21f3bbd882f3a5a30c58db6ff/t-kIKFAh/test-project-2/packages/test-app/

      30 |     const index = file.indexOf(NODE_MODULES_PATTERN)
      31 |     if (index < 0) {
    > 32 |       throw new Error(`${file} must be under ${srcWithEndSlash}`)
         |             ^
      33 |     } else {
      34 |       return file.substring(index + 1 /* leading slash */)
      35 |     }

      at getRelativePath (../packages/app-builder-lib/src/util/filter.ts:32:13)
      at AsarPackager.unpackPattern (../packages/app-builder-lib/src/util/filter.ts:57:20)
      at AsarPackager.createPackageFromFiles (../packages/app-builder-lib/src/asar/asarUtil.ts:129:82)
      at AsarPackager.pack (../packages/app-builder-lib/src/asar/asarUtil.ts:49:41)
      at ../packages/app-builder-lib/src/platformPackager.ts:439:11
          at async Promise.all (index 0)
      at AsyncTaskManager.awaitTasks (../packages/builder-util/src/asyncTaskManager.ts:65:25)
      at LinuxPackager.doPack (../packages/app-builder-lib/src/platformPackager.ts:293:5)
      at LinuxPackager.pack (../packages/app-builder-lib/src/platformPackager.ts:138:5)
      at Packager.doBuild (../packages/app-builder-lib/src/packager.ts:459:9)
      at executeFinally (../packages/builder-util/src/promise.ts:12:14)
      at Packager.build (../packages/app-builder-lib/src/packager.ts:393:31)
      at packAndCheck (src/helpers/packTester.ts:188:41)
      at src/helpers/packTester.ts:132:36
      at executeFinally (../packages/builder-util/src/promise.ts:12:14)

Currently, the returned node_modules paths are no longer symlinks, but resolved paths. For example, packages in pnpm workspace/yarn workspace are not under node_modules.

All previous getRealSource/getRelativePath methods were based on node_modules in the path, which was relatively tricky.

const getRealSource = (name: string, source: string) => {
let parentDir = path.dirname(source)
const scopeDepth = name.split("/").length
// get the parent dir of the package, input: /root/path/node_modules/@electron/remote, output: /root/path/node_modules
for (let i = 0; i < scopeDepth - 1; i++) {
parentDir = path.dirname(parentDir)
}
// for the local node modules which is not in node modules
if (!parentDir.endsWith(path.sep + NODE_MODULES)) {
return parentDir
}

function getRelativePath(file: string, srcWithEndSlash: string) {
if (!file.startsWith(srcWithEndSlash)) {
const index = file.indexOf(NODE_MODULES_PATTERN)
if (index < 0) {
throw new Error(`${file} must be under ${srcWithEndSlash}`)
} else {
return file.substring(index + 1 /* leading slash */)
}
}

Solution: Directly include relativeNodeModulesPath in the file stats. This is simple and easy to understand.

Copy link

changeset-bot bot commented Oct 9, 2024

🦋 Changeset detected

Latest commit: a2871a1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages
Name Type
app-builder-lib Patch
builder-util Patch
dmg-builder Patch
electron-builder-squirrel-windows Patch
electron-builder Patch
electron-forge-maker-appimage Patch
electron-forge-maker-nsis-web Patch
electron-forge-maker-nsis Patch
electron-forge-maker-snap Patch
electron-publish Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@beyondkmp beyondkmp marked this pull request as draft October 9, 2024 13:50
Copy link

netlify bot commented Oct 9, 2024

Deploy Preview for car-park-attendant-cleat-11576 ready!

Name Link
🔨 Latest commit a2871a1
🔍 Latest deploy log https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/6707e7a0553b770008cca2eb
😎 Deploy Preview https://deploy-preview-8576--car-park-attendant-cleat-11576.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@beyondkmp beyondkmp marked this pull request as ready for review October 9, 2024 14:40
@beyondkmp beyondkmp requested a review from mmaietta October 9, 2024 14:40
@beyondkmp beyondkmp changed the title fix: workspace asarupack issue fix: packages in the workspace not being under node_modules Oct 9, 2024
@beyondkmp beyondkmp merged commit 3eab714 into electron-userland:master Oct 10, 2024
13 checks passed
@beyondkmp beyondkmp deleted the workspace branch October 10, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make symlink behavior configurable
2 participants