Update Electron to latest version 33.3.1 #782

nightnei · 2025-01-07T15:05:32Z

Related issues

Fixes https://github.com/Automattic/dotcom-forge/issues/9631

Proposed Changes

With this PR we are updating Electron to the latest version to keep it up-to-date.

Testing Instructions

Run Studio locally npm start
Assert that there are no (new) errors/warning
Open Studio
Select Electron in the app's menu and then Test Render Failure (dev only)
Assert that you still see the same expected error (here were changes in code of this PR)
Smoke test Studio

wojtekn

The change looks clear, and the Studio still works fine. I've checked dev build, too, and it installs and runs correctly.

We may upgrade electron-forge at some point, too, but we will need to review issues with version format change we spotted last time we tried.

wojtekn

Actually, it seems I can't import Jetpack backup to a Studio site anymore as I'm getting the error:

Error occurred in handler for 'importSite': TypeError: Cannot read properties of undefined (reading 'endsWith')
    at /Users/cyphelf/Sites/studio/.webpack/main/index.js:227050:58
    at Array.some (<anonymous>)
    at BackupHandlerFactory.isTarGz (/Users/cyphelf/Sites/studio/.webpack/main/index.js:227050:34)
    at BackupHandlerFactory.create (/Users/cyphelf/Sites/studio/.webpack/main/index.js:227034:23)
    at /Users/cyphelf/Sites/studio/.webpack/main/index.js:227583:81
    at Generator.next (<anonymous>)
    at fulfilled (/Users/cyphelf/Sites/studio/.webpack/main/index.js:227546:58)

nightnei · 2025-01-08T16:15:02Z

@wojtekn

Actually, it seems I can't import Jetpack backup to a Studio site anymore as I'm getting the error:

Thank you, fixed

We may upgrade electron-forge at some point, too, but we will need to review issues with version format change we spotted last time we tried.

I can take a look at it. Do we have discussion somewhere to read and get familiar with background and previous attempts.

wojtekn · 2025-01-08T16:56:49Z

Thank you, fixed

Thanks, I will run through the whole smoke test suite.

I can take a look at it. Do we have discussion somewhere to read and get familiar with background and previous attempts.

Sounds great! See #201 for the context and feel free to open a separate issue for that.

fredrikekelund

I don't have any decisive feedback on this yet. I'll take another look tomorrow, but @wojtekn might beat me to approving it.

Should we hold off with merging this until the 1.3.2 release to have some more time to test it..?

fredrikekelund · 2025-01-09T15:41:04Z

src/menu.ts

+				withMainWindow( ( window ) => {
+					window.webContents.send( 'test-render-failure' );
+				} );


Out of curiosity, which change in Electron made it so we had to make this change?

They replaced BrowserWindow with BaseWindow, so in this argument we don't have access to webContents anymore.

@fredrikekelund https://github.com/electron/electron/pull/44953/files

fredrikekelund · 2025-01-09T15:53:26Z

src/hooks/use-import-export.tsx

@@ -101,9 +101,16 @@ export const ImportExportProvider = ( { children }: { children: React.ReactNode
 			try {
 				await stopServer( selectedSite.id );

+				let filePath: string;
+				if ( file instanceof File ) {
+					filePath = getIpcApi().showFilePath( file );


The Electron docs led me to believe that we can access webUtils.getPathForFile directly in this file, but that's clearly not the case when I test it…

How should we understand it when the Electron docs say Process: Renderer..? cc @Automattic/yolo

I think it's mistake in documentation. After a few days of working with Electron documentation and Issues I noticed that it's not rare case of mistakes in docs.
It should not be in Renderer for sure, since the intention of webUtils.getPathForFile is to move retrieving path logic from Renderer to Preload layer.

fredrikekelund · 2025-01-09T15:54:31Z

src/hooks/use-import-export.tsx

@@ -64,7 +64,7 @@ export const ImportExportProvider = ( { children }: { children: React.ReactNode

 	const importFile = useCallback(
 		async (
-			file: BackupArchiveInfo,
+			file: File | BackupArchiveInfo,


Why did this work previously? 🤔 Because File and BackupArchiveInfo both have a path prop?

Long story short - If it walks like a duck and quacks like a duck, then it probably is a duck 😂

Will try to explain step by step, maybe it will be easier to understand:

BackupArchiveInfo is name and path properties

Inside of importFile we also expect only name and path

So TS doesn't care which extra properties of the object we provide and what the object actualy is. Since anyway we use only these two properties.

I just mean that we clearly passed a File instance to importFile previously, too (see below). It's curious that TS didn't bark at us for doing that 🤔

studio/src/hooks/use-add-site.ts

Lines 61 to 64 in 51a4aa2

await importFile( fileForImport, newSite, {

showImportNotification: false,

isNewSite: true,

} );

I see what you mean.
Yeah, as I mentioned before, it's about "duck typing".
File is just set of properties, so as soon as we provide all expected properties - it's enough for TS to be happy 😄
If you curious, I made a quick example for you to play with it.

Gotcha 👍 Thanks for the clarification!

wojtekn · 2025-01-10T08:02:27Z

I don't have any decisive feedback on this yet. I'll take another look tomorrow, but @wojtekn might beat me to approving it.
Should we hold off with merging this until the 1.3.2 release to have some more time to test it..?

I think we should run the whole smoke test suite on Mac and Windows before we merge.

fredrikekelund

I didn't encounter any issues while testing the following stuff:

creating a new blank site
creating a new site from a Studio export
creating a new site from a Playground export
importing a Studio export to an existing site
importing a database into an existing site
creating a demo site
pulling a site
pushing a site
chatting with Assistant
changing the PHP version
deleting a site

I left a few remarks on the code, but still approving the PR based on my smoke testing on Mac.

Per @wojtekn's latest comment, we should also run a similar test suite on Windows before merging.

fredrikekelund · 2025-01-10T14:27:12Z

src/ipc-types.d.ts

+	 * webUtils.getPathForFile is available only inside preload script, that's why this one function is exception and need to be defined here manually.
+	 *
+	 * The nonstandard `path` property of the Web `File` object was added in an early version of Electron as a convenience method for working with native files when doing everything in the renderer was more common.
+	 * However, it represents a deviation from the standard and poses a minor security risk as well, so beginning in Electron 32.0 it has been removed in favor of the [`webUtils.getPathForFile`](api/web-utils.md#webutilsgetpathforfilefile) method.


Suggested change

* webUtils.getPathForFile is available only inside preload script, that's why this one function is exception and need to be defined here manually.

*

* The nonstandard `path` property of the Web `File` object was added in an early version of Electron as a convenience method for working with native files when doing everything in the renderer was more common.

* However, it represents a deviation from the standard and poses a minor security risk as well, so beginning in Electron 32.0 it has been removed in favor of the [`webUtils.getPathForFile`](api/web-utils.md#webutilsgetpathforfilefile) method.

* `webUtils.getPathForFile` is available only inside preload script, that's why this one

* function is exception and need to be defined here manually.

*

* See https://www.electronjs.org/docs/latest/breaking-changes#planned-breaking-api-changes-320

* for more details.

Nit, but I would shorten this and refer directly to the Electron docs.

fredrikekelund · 2025-01-10T14:29:06Z

src/hooks/use-import-export.tsx

+				let filePath: string;
+				if ( file instanceof File ) {
+					filePath = getIpcApi().showFilePath( file );
+				} else {


Maybe we should move this logic to src/hooks/use-add-site.ts instead? Seems reasonable that this function would consume a single format, and src/hooks/use-add-site.ts already calls the IPC API in some other places, so it wouldn't be exotic to also introduce a getIpcApi().showFilePath call

It's really reasonable idea, but it's used more than in src/hooks/use-add-site.ts. There are 6 calls of this function, and that's why I decided to keep it flexible with expecting bothFile | { path: ... }. Otherwise, it would be just copy-paste. WDYT about it?

I think src/hooks/use-add-site.ts is the only place where we pass a File instance, though.

I double-checked - it's 2 files, not 3. Also content-tab-import-export

fredrikekelund · 2025-01-10T14:30:05Z

src/preload.ts

@@ -101,6 +101,7 @@ const api: IpcApi = {
 		ipcRenderer.invoke( 'getConnectedWpcomSites', localSiteId ),
 	addSyncOperation: ( id: string ) => ipcRenderer.invoke( 'addSyncOperation', id ),
 	clearSyncOperation: ( id: string ) => ipcRenderer.invoke( 'clearSyncOperation', id ),
+	showFilePath: webUtils.getPathForFile,


Suggested change

showFilePath: webUtils.getPathForFile,

getPathForFile: webUtils.getPathForFile,

I don't think showFilePath is an ideal name. Something with "get" would be better, since the function is responsible for retrieving a string

I was thinking about it too, but decided to take this name, since it was somewhere in documentation, don't remember where exactly I saw it. It's hard to provide some arguments or explain my thoughts, but I will try 😅
This name is provided by creators of Electron, so maybe it will be kinda-ish following style guides for naming. I just want to believe, that they were thinking about something, when gave this name to "expose it to main world".
Totally not strong arguments, it's more about my feelings 😅 So I am ok to go with either name, just decided to explain my decision 🙂

Also, if we decided to use something else instead of showFilePath - I think it's better to chose whatever, but not getPathForFile , to simplify Command + F across our codebase, avoid confusion and simplify reading code at a glance.

if we decided to use something else instead of showFilePath - I think it's better to chose whatever, but not getPathForFile , to simplify Command + F across our codebase, avoid confusion and simplify reading code at a glance

I actually think that reusing the getPathForFile name makes sense since we are simply exposing that API without any modifications.

* chore(package.json): update electron-forge to latest version 7.6.1 * chore: space * chore: test removing overrides for electron-packager * chore: revert testing broken Windows

fredrikekelund · 2025-01-15T10:32:20Z

src/hooks/use-import-export.tsx

+				let filePath: string;
+				if ( file instanceof File ) {
+					filePath = getIpcApi().getPathForFile( file );
+				} else {
+					filePath = file.path;
+				}


Suggested change

let filePath: string;

if ( file instanceof File ) {

filePath = getIpcApi().getPathForFile( file );

} else {

filePath = file.path;

}

const filePath = file instanceof File ? getIpcApi().getPathForFile( file ) : file.path;

We could make this slightly more compact

nightnei added 2 commits January 7, 2025 15:02

chore(electron): update to latest version 33.3.1

68ecf5f

chore: fix node-abi

5279229

nightnei self-assigned this Jan 7, 2025

nightnei requested a review from sejas January 7, 2025 17:32

wojtekn approved these changes Jan 8, 2025

View reviewed changes

wojtekn changed the title ~~chore(electron): update to latest version 33.3.1~~ Update Electron to latest version 33.3.1 Jan 8, 2025

wojtekn self-requested a review January 8, 2025 09:48

wojtekn requested changes Jan 8, 2025

View reviewed changes

nightnei added 2 commits January 8, 2025 15:11

fix: importing files

16f0f12

fix: file type

2cd9c4d

fredrikekelund reviewed Jan 9, 2025

View reviewed changes

fredrikekelund approved these changes Jan 10, 2025

View reviewed changes

nightnei added 2 commits January 13, 2025 22:16

chore: comment

f55c5b7

Studio: Update electron-forge to the latest version 7.6.0 (#795)

6c88103

* chore(package.json): update electron-forge to latest version 7.6.1 * chore: space * chore: test removing overrides for electron-packager * chore: revert testing broken Windows

wojtekn self-requested a review January 14, 2025 08:58

wojtekn approved these changes Jan 15, 2025

View reviewed changes

chore: renaming

357a5a5

fredrikekelund reviewed Jan 15, 2025

View reviewed changes

chore: simplify

7976c9e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Electron to latest version 33.3.1 #782

Update Electron to latest version 33.3.1 #782

nightnei commented Jan 7, 2025 •

edited

Loading

wojtekn left a comment

wojtekn left a comment

nightnei commented Jan 8, 2025 •

edited

Loading

wojtekn commented Jan 8, 2025

fredrikekelund left a comment

fredrikekelund Jan 9, 2025

nightnei Jan 9, 2025 •

edited

Loading

nightnei Jan 9, 2025

fredrikekelund Jan 9, 2025

nightnei Jan 9, 2025

fredrikekelund Jan 9, 2025

nightnei Jan 9, 2025

fredrikekelund Jan 10, 2025

nightnei Jan 10, 2025 •

edited

Loading

fredrikekelund Jan 10, 2025

wojtekn commented Jan 10, 2025

fredrikekelund left a comment

fredrikekelund Jan 10, 2025

fredrikekelund Jan 10, 2025

nightnei Jan 13, 2025

fredrikekelund Jan 14, 2025

nightnei Jan 15, 2025

fredrikekelund Jan 10, 2025

nightnei Jan 13, 2025

fredrikekelund Jan 14, 2025

fredrikekelund Jan 15, 2025

	await importFile( fileForImport, newSite, {
	showImportNotification: false,
	isNewSite: true,
	} );

-	 * webUtils.getPathForFile is available only inside preload script, that's why this one function is exception and need to be defined here manually.
-	 *
-	 * The nonstandard `path` property of the Web `File` object was added in an early version of Electron as a convenience method for working with native files when doing everything in the renderer was more common.
-	 * However, it represents a deviation from the standard and poses a minor security risk as well, so beginning in Electron 32.0 it has been removed in favor of the [`webUtils.getPathForFile`](api/web-utils.md#webutilsgetpathforfilefile) method.
+	 * `webUtils.getPathForFile` is available only inside preload script, that's why this one
+	 * function is exception and need to be defined here manually.
+	 *
+	 * See https://www.electronjs.org/docs/latest/breaking-changes#planned-breaking-api-changes-320
+	 * for more details.

	showFilePath: webUtils.getPathForFile,
	getPathForFile: webUtils.getPathForFile,

Update Electron to latest version 33.3.1 #782

Are you sure you want to change the base?

Update Electron to latest version 33.3.1 #782

Conversation

nightnei commented Jan 7, 2025 • edited Loading

Related issues

Proposed Changes

Testing Instructions

wojtekn left a comment

Choose a reason for hiding this comment

wojtekn left a comment

Choose a reason for hiding this comment

nightnei commented Jan 8, 2025 • edited Loading

wojtekn commented Jan 8, 2025

fredrikekelund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightnei Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightnei Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtekn commented Jan 10, 2025

fredrikekelund left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightnei commented Jan 7, 2025 •

edited

Loading

nightnei commented Jan 8, 2025 •

edited

Loading

nightnei Jan 9, 2025 •

edited

Loading

nightnei Jan 10, 2025 •

edited

Loading