feat: App builder bin 5.0 #8147

beyondkmp · 2024-03-22T12:05:54Z

No description provided.

changeset-bot · 2024-03-22T12:05:59Z

🦋 Changeset detected

Latest commit: 7f73cd2

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

netlify · 2024-03-22T12:06:10Z

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

Name	Link
🔨 Latest commit	`7f73cd2`
🔍 Latest deploy log	https://app.netlify.com/sites/car-park-attendant-cleat-11576/deploys/660241705b98940007509b1a
😎 Deploy Preview	https://deploy-preview-8147--car-park-attendant-cleat-11576.netlify.app
📱 Preview on mobile	Toggle 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 · 2024-03-23T17:20:43Z

@mmaietta Please help review when you have time.

mmaietta · 2024-03-24T01:23:19Z

packages/app-builder-lib/src/util/appFileCopier.ts

+  if (pnpmIndex >= 0 && parts.length > pnpmIndex + 2) {
+    // Remove '.pnpm' and the next two folders from the parts array
+    parts.splice(pnpmIndex, 3)
+  }


Are we able to make this guarantee that the two nested folders within .pnpm folder are okay to be removed? Are they unnecessary symlinks or something?

beyondkmp · 2024-04-03T02:47:03Z

I have a question about conflict versions 2.
|------ ms 2.1.1
test-app---
|------ foo 1.0.0 ----| --- ms 2.0.0

The expected result is

exports[`conflict versions 2`] = `
Object {
  "files": Object {
    "index.html": Object {
      "offset": "9018",
      "size": 841,
    },
    "index.js": Object {
      "offset": "9859",
      "size": 2501,
    },
    "node_modules": Object {
      "files": Object {
        "foo": Object {
          "files": Object {
            "package.json": Object {
              "offset": "4310",
              "size": 127,
            },
          },
        },
        "ms": Object {
          "files": Object {
            "index.js": Object {
              "offset": "4437",
              "size": 3034,
            },
            "license.md": Object {
              "offset": "7471",
              "size": 1077,
            },
            "package.json": Object {
              "offset": "8548",
              "size": 470,
            },
          },
        },
      },
    },
    "package.json": Object {
      "offset": "12360",
      "size": 301,
    },
  },
}
`;

I think the expected result is wrong, the right expected result is following:

exports[`conflict versions 2`] = `
Object {
  "files": Object {
    "index.html": Object {
      "offset": "9018",
      "size": 841,
    },
    "index.js": Object {
      "offset": "9859",
      "size": 2501,
    },
    "node_modules": Object {
      "files": Object {
        "foo": Object {
          "files": Object {
            "package.json": Object {
              "offset": "4310",
              "size": 127,
            },
          },
           "files": Object {
            "node_modules": Object {
                  "ms": Object {
                 "files": Object {
                    "index.js": Object {
                      "offset": "4437",
                      "size": 3034,
                    },
                    "license.md": Object {
                      "offset": "7471",
                      "size": 1077,
                    },
                    "package.json": Object {
                      "offset": "8548",
                      "size": 470,
                    },
                  },
                },
            },
          },
        },
        "ms": Object {
          "files": Object {
            "index.js": Object {
              "offset": "4437",
              "size": 3034,
            },
            "license.md": Object {
              "offset": "7471",
              "size": 1077,
            },
            "package.json": Object {
              "offset": "8548",
              "size": 470,
            },
          },
        },
      },
    },
    "package.json": Object {
      "offset": "12360",
      "size": 301,
    },
  },
}
`;

@mmaietta What do you think? When dealing with conflict versions before, did the electron-buidler only keep one version?

mmaietta · 2024-04-03T16:44:29Z

It looks like we're copying the same assets twice now, but I find it odd that the offset and size values are the same. AFAICT, the packaged app size shouldn't be changed since the total offset is still the same for the asar. As such, I think just updating the snapshot should be fine and we can proceed with the PR

mmaietta · 2024-04-03T16:46:31Z

test/snapshots/HoistedNodeModuleTest.js.snap

@@ -22,7 +22,7 @@ Object {
        "foo": Object {
          "files": Object {
            "package.json": Object {
-              "offset": "0",
+              "offset": "4310",


This is an odd change. What's at offset 0 now?

mmaietta · 2024-04-03T16:48:11Z

packages/app-builder-lib/src/util/appFileCopier.ts

+  const destinationParts: string[] = destination.split(path.sep)
+
+  const nodeModulesIndicesFilePath: number[] = filePathParts.reduce((acc: number[], part: string, index: number) => {
+    if (part === "node_modules") acc.push(index)


Minor: Please move this to new line

if (part === "node_modules") { acc.push(index) }

Same comment applies to line 41

beyondkmp · 2024-04-03T22:24:05Z

It looks like we're copying the same assets twice now, but I find it odd that the offset and size values are the same. AFAICT, the packaged app size shouldn't be changed since the total offset is still the same for the asar. As such, I think just updating the snapshot should be fine and we can proceed with the PR

You might have misunderstood. My example above was just that, directly copying data from MS modules. The approach of electron-builder is to copy two different versions of MS to the same directory, with the last copied version overriding the previous ones.

I think there might be some issues with this approach, but it seems like no one has ever reported a problem.

mmaietta · 2024-04-03T23:49:26Z

Ohh, interesting! I was curious as to how electron-builder has handled conflicting versions. Agreed, no one has ever reported a problem over the few years I've managed this project, so we may be able to proceed with your suggested/expected result as-is as it seems like it'd be the correct approach to me.

beyondkmp · 2024-04-09T10:19:29Z

Ohh, interesting! I was curious as to how electron-builder has handled conflicting versions. Agreed, no one has ever reported a problem over the few years I've managed this project, so we may be able to proceed with your suggested/expected result as-is as it seems like it'd be the correct approach to me.

Under the current implementation, this is not achievable. I have made modifications to the app-builder's implementation to achieve this. Additionally, after making these changes, we no longer need to handle pnmp separately.

Taking test-app-yarn-workspace-version-conflict as an example, the old app-builder output format was like this:

[
	{
		"dir": "~\\app-builder\\pkg\\node-modules\\yarn-demo\\node_modules",
		"deps": [
			{
				"name": "ms",
				"version": "2.0.0"
			}
		]
	},
	{
		"dir": "~\\app-builder\\pkg\\node-modules\\yarn-demo\\packages\\test-app\\node_modules",
		"deps": [
			{
				"name": "foo",
				"version": "1.0.0"
			},
			{
				"name": "ms",
				"version": "2.1.1"
			}
		]
	}
]

After the changes, the output format is like this:

[
	{
		"name": "foo",
		"version": "1.0.0",
		"dir": "~\\app-builder\\pkg\\node-modules\\yarn-demo\\packages\\foo",
		"conflictDependency": [
			{
				"name": "ms",
				"version": "2.0.0",
				"dir": "~\\app-builder\\pkg\\node-modules\\yarn-demo\\node_modules\\ms"
			}
		]
	},
	{
		"name": "ms",
		"version": "2.1.1",
		"dir": "~\\app-builder\\pkg\\node-modules\\yarn-demo\\packages\\test-app\\node_modules\\ms"
	}
]

Another exmaple - pnpm-demo:
old format:

[
	{
		"dir": "~\\app-builder\\pkg\\node-modules\\pnpm-demo\\node_modules",
		"deps": [
			{
				"name": "react",
				"version": "18.2.0"
			}
		]
	},
	{
		"dir": "~\\app-builder\\pkg\\node-modules\\pnpm-demo\\node_modules\\.pnpm\\loose-envify@1.4.0\\node_modules",
		"deps": [
			{
				"name": "js-tokens",
				"version": "4.0.0"
			}
		]
	},
	{
		"dir": "~\\app-builder\\pkg\\node-modules\\pnpm-demo\\node_modules\\.pnpm\\react@18.2.0\\node_modules\\react\\node_modules",
		"deps": [
			{
				"name": "loose-envify",
				"version": "1.4.0"
			}
		]
	}
]

new format:

[
	{
		"name": "js-tokens",
		"version": "4.0.0",
		"dir": "~\\app-builder\\pkg\\node-modules\\pnpm-demo\\node_modules\\.pnpm\\loose-envify@1.4.0\\node_modules\\js-tokens"
	},
	{
		"name": "loose-envify",
		"version": "1.4.0",
		"dir": "~\\app-builder\\pkg\\node-modules\\pnpm-demo\\node_modules\\.pnpm\\react@18.2.0\\node_modules\\loose-envify"
	},
	{
		"name": "react",
		"version": "18.2.0",
		"dir": "~\\app-builder\\pkg\\node-modules\\pnpm-demo\\node_modules\\react"
	}
]

In the new format, it's clearer to see which packages are depended on and which ones are conflicting.

beyondkmp · 2024-04-09T10:27:25Z

I have already integrated the new implementation method in this PR(beyondkmp#2) and passed all the tests. When you have time, please review it. If there are no issues, I can first submit a PR to app-builder and then submit this PR to electron-builder.

beyondkmp · 2024-04-28T07:42:54Z

I have already integrated the new implementation method in this PR(beyondkmp#2) and passed all the tests. When you have time, please review it. If there are no issues, I can first submit a PR to app-builder and then submit this PR to electron-builder.

Here's app-builder PR(develar/app-builder#116)

beyondkmp · 2024-04-30T04:36:30Z

The app-builder PR(develar/app-builder#116) is merged. Please help publish a alpha build(v5.0.0-alpha.3) for app-builder.

mmaietta · 2024-05-01T18:12:47Z

Done https://github.com/develar/app-builder/releases/tag/v5.0.0-alpha.3

beyondkmp · 2024-05-01T21:30:10Z

closed as #8190.

PayneFuRC added 2 commits March 22, 2024 19:11

update app-builder-bin-5.0

3c857ba

fix pnpm modules

ce3e7c3

github-actions bot added mac app-builder-lib builder-util labels Mar 22, 2024

update ut

341cb13

github-actions bot removed the mac label Mar 22, 2024

PayneFuRC added 6 commits March 22, 2024 22:27

find the not found

b63495c

fix ut

5cc4902

delete log

8d7aa73

delete useless code

2c06361

delete log

62a5d55

merge getDestination

e2af888

mmaietta reviewed Mar 24, 2024

View reviewed changes

mmaietta and others added 3 commits March 24, 2024 13:43

Create wet-pears-doubt.md

5bbf0f6

format error message

269e837

delete comments

7f73cd2

mmaietta reviewed Apr 3, 2024

View reviewed changes

beyondkmp closed this May 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: App builder bin 5.0 #8147

feat: App builder bin 5.0 #8147

beyondkmp commented Mar 22, 2024

changeset-bot bot commented Mar 22, 2024 •

edited

netlify bot commented Mar 22, 2024 •

edited

beyondkmp commented Mar 23, 2024 •

edited

mmaietta Mar 24, 2024

beyondkmp commented Apr 3, 2024 •

edited

mmaietta commented Apr 3, 2024

mmaietta Apr 3, 2024

mmaietta Apr 3, 2024

beyondkmp commented Apr 3, 2024

mmaietta commented Apr 3, 2024

beyondkmp commented Apr 9, 2024 •

edited

beyondkmp commented Apr 9, 2024

beyondkmp commented Apr 28, 2024

beyondkmp commented Apr 30, 2024

mmaietta commented May 1, 2024

beyondkmp commented May 1, 2024

feat: App builder bin 5.0 #8147

feat: App builder bin 5.0 #8147

Conversation

beyondkmp commented Mar 22, 2024

changeset-bot bot commented Mar 22, 2024 • edited

🦋 Changeset detected

netlify bot commented Mar 22, 2024 • edited

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

beyondkmp commented Mar 23, 2024 • edited

mmaietta Mar 24, 2024

Choose a reason for hiding this comment

beyondkmp commented Apr 3, 2024 • edited

mmaietta commented Apr 3, 2024

mmaietta Apr 3, 2024

Choose a reason for hiding this comment

mmaietta Apr 3, 2024

Choose a reason for hiding this comment

beyondkmp commented Apr 3, 2024

mmaietta commented Apr 3, 2024

beyondkmp commented Apr 9, 2024 • edited

beyondkmp commented Apr 9, 2024

beyondkmp commented Apr 28, 2024

beyondkmp commented Apr 30, 2024

mmaietta commented May 1, 2024

beyondkmp commented May 1, 2024

changeset-bot bot commented Mar 22, 2024 •

edited

netlify bot commented Mar 22, 2024 •

edited

beyondkmp commented Mar 23, 2024 •

edited

beyondkmp commented Apr 3, 2024 •

edited

beyondkmp commented Apr 9, 2024 •

edited