Desktop: Bundle default plugins with desktop application #6679

mak2002 · 2022-07-19T16:11:22Z

Summary

This PR implements bundling plugins with desktop app. When the app is being built, we will run a JS file that pulls the jpl files from the NPM registry and then moves it in build folder of the app. In this script, we are providing default plugins IDs with their versions that we intend to use.

CalebJohn · 2022-07-20T07:29:21Z

.github/scripts/run_ci.sh

+echo "Step: Bundling default plugins in desktop application"
+cd "$ROOT_DIR/packages/tools"
+tsc bundleDefaultPlugins.ts
+node bundleDefaultPlugins.js


This code is only necessary when building the desktop app, but you have it here where it will be called even when building the server image. I think this code only needs to be run for the first block below (Building the desktop application for publishing).

I will move it then. I thought Docker Image also needs these plugins.

laurent22 · 2022-07-28T15:48:07Z

.github/scripts/run_ci.sh

+ tsc bundleDefaultPlugins.ts
+ node bundleDefaultPlugins.js


yarn run build runs before getting here so this TS file should already been compiled, so please remove the tsc statement

laurent22 · 2022-07-28T15:51:45Z

packages/tools/bundleDefaultPlugins.test.ts

+import fs = require('fs-extra');
+const fetch = require('node-fetch');
+
+describe('bundlePlugins', function() {


Should be named like the file bundleDefaultPlugins

laurent22 · 2022-08-08T15:18:55Z

packages/tools/bundleDefaultPlugins.test.ts

+describe('bundlePlugins', function() {
+
+ beforeEach(() => {
+ jest.setTimeout(20000);


What is this setTimeout for?

I am using this just to increase the timeout for tests on my local system, otherwise they seem to fail randomly. This will be removed before merging the code.

laurent22 · 2022-07-28T15:53:32Z

packages/tools/bundleDefaultPlugins.test.ts

+
+ let manifests = [];
+ try {
+ const manifestData = await fetch('https://raw.githubusercontent.com/joplin/plugins/master/manifests.json');


Is it actually downloading stuff off GitHub? We can't have this, you'll need to mock this. In general all tests should be able to run offline.

laurent22 · 2022-07-28T15:54:21Z

packages/tools/bundleDefaultPlugins.test.ts

+ try {
+ const manifestData = await fetch('https://raw.githubusercontent.com/joplin/plugins/master/manifests.json');
+ manifests = JSON.parse(await manifestData.text());
+ if (!manifests) throw new Error('Invalid or missing JSON');
+ } catch (error) {
+ console.log(error);
+ }


Something's not right here, it shouldn't log anything in tests. You need to test that it doesn't throw, if it's not supposed to throw. And in fact I think the tests themselves shouldn't throw exception (the code they call might).

laurent22 · 2022-07-28T15:55:50Z

packages/tools/bundleDefaultPlugins.ts

+const downloadFile = async (url: string, name: string) => {
+ const response = await fetch(url);
+ if (!response.ok) throw new Error(`Unexpected response ${response.statusText}`);
+ await streamPipeline(response.body, fs.createWriteStream(name));


Is it not possible to use fetch? Too many problems with streams in general

I think we can't use fetch alone, we have to write the response to a file. If we don't want to use streams then we can use the following alternative:

function downloadFile(url:string, outputPath:string) { return fetch(url) .then(response => response.buffer()) .then(buff => writeFilePromise(outputPath, Buffer.from(buff))); }

laurent22 · 2022-08-08T15:18:27Z

packages/tools/bundleDefaultPlugins.test.ts

+
+import path = require('path');
+import { downloadPlugins, localPluginsVersion } from './bundleDefaultPlugins';
+import fs = require('fs-extra');


Don't import * only import the functions you need

laurent22 · 2022-08-08T15:18:33Z

packages/tools/bundleDefaultPlugins.test.ts

+
+
+
+import path = require('path');


only import the functions you need

laurent22 · 2022-08-08T15:18:55Z

packages/tools/bundleDefaultPlugins.test.ts

+describe('bundlePlugins', function() {
+
+ beforeEach(() => {
+ jest.setTimeout(20000);


What is this setTimeout for?

laurent22 · 2022-08-08T15:19:16Z

packages/tools/bundleDefaultPlugins.test.ts

+ await downloadPlugins(tempDir, localPluginsVersions, testDefaultPluginsIds, manifests);
+
+ expect(fs.existsSync(`${tempDir}/io.github.jackgruber.backup`)).toBe(true);
+ expect(fs.existsSync(`${tempDir}/plugin.calebjohn.rich-markdown`)).toBe(true);


Only use async methods

laurent22 · 2022-08-08T15:19:58Z

packages/tools/bundleDefaultPlugins.ts

+const fetch = require('node-fetch');
+const { writeFile } = require('fs');
+const { promisify } = require('util');
+const writeFilePromise = promisify(writeFile);


just import from fs-extra, it's already promisified

laurent22 · 2022-08-08T15:20:17Z

packages/tools/bundleDefaultPlugins.ts

+ localPluginsVersions[pluginId] = '0.0.0';
+ continue;
+ }
+ const data = fs.readFileSync(`${defaultPluginDir}/${pluginId}/manifest.json`, 'utf8');


only async calls

laurent22 · 2022-08-08T15:20:31Z

packages/tools/bundleDefaultPlugins.ts

+};
+
+function downloadFile(url: string, outputPath: string) {
+ return fetch(url).then(response => response.buffer()).then(buff => writeFilePromise(outputPath, Buffer.from(buff)));


no then, only await/async

laurent22 · 2022-08-08T15:20:45Z

packages/tools/bundleDefaultPlugins.ts

+ const pluginName = `${manifests[pluginId]._npm_package_name}-${defaultPluginsId[pluginId]}.tgz`;
+ await downloadFile(pluginUrl, pluginName);
+
+ if (!fs.existsSync(pluginName)) throw new Error(`${pluginName} cannot be downloaded`);


only async code

CalebJohn · 2022-08-09T14:55:23Z

.github/scripts/run_ci.sh

@@ -170,6 +170,8 @@ cd "$ROOT_DIR/packages/app-desktop"

 if [[ $GIT_TAG_NAME = v* ]]; then
 echo "Step: Building and publishing desktop application..."
+ cd "$ROOT_DIR/packages/tools"


Line 175 Is meant to be run form the packages/app-desktop directory, but this line places the script in packages/tools. This appears like it will break the ci build (for releases).

Ok. I will fix it.

…/joplin into bundle-default-plugins-1

laurent22

Nearly there I think, just a few more tweaks

laurent22 · 2022-08-27T11:22:34Z

packages/tools/bundleDefaultPlugins.ts

+ }
+}
+
+export const downloadPlugins = async (localPluginsVersions: PluginAndVersion, defaultPluginsInfo: DefaultPluginsInfo, manifests: any): Promise<any> => {


Shouldn't return any

Updated this with a type.

laurent22 · 2022-08-27T11:23:37Z

packages/tools/bundleDefaultPlugins.ts

+ const pluginName = `${manifests[pluginId]._npm_package_name}-${defaultPluginsInfo[pluginId].version}.tgz`;
+ await downloadFile(pluginUrl, pluginName);
+
+ if (!await pathExists(pluginName)) throw new Error(`${pluginName} cannot be downloaded`);


You don't need this check, because downloadFile should throw if there's an error

Yes. Thanks for catching this.

packages/tools/bundleDefaultPlugins.ts

…/joplin into bundle-default-plugins-1

laurent22 · 2022-08-28T14:39:49Z

packages/tools/bundleDefaultPlugins.ts


 const downloadedPluginsNames: PluginIdAndName = {};
 for (const pluginId of Object.keys(defaultPluginsInfo)) {
 if (localPluginsVersions[pluginId] === defaultPluginsInfo[pluginId].version) continue;
 const response = await fetch(`https://registry.npmjs.org/${manifests[pluginId]._npm_package_name}`);

 if (!response.ok) {
- const responseText = await response.text();
- throw new Error(`Cannot download plugin file ${responseText.substr(0,500)}`);
+ throw new Error(`Cannot fetch ${manifests[pluginId]._npm_package_name} release info from NPM`);


Actually your previous code was good, as you were showing the actual error message. Any reason you removed it?

Actually, while testing locally I thought that .text() will show buffer data but on failing to fetch it won't have any buffer data to show 😅 . Restored the previous message.

laurent22 · 2022-08-28T14:40:35Z

packages/tools/bundleDefaultPlugins.ts

@@ -32,10 +32,13 @@ export const localPluginsVersion = async (defaultPluginDir: string, defaultPlugi

 async function downloadFile(url: string, outputPath: string) {
 const response = await fetch(url);
+ if (!response.ok) {
+ throw new Error(`Cannot download file from ${url}`);


Here too you might want to provide more info such as response.status or response.text

laurent22 · 2022-09-01T10:53:51Z

Looks good now, thanks @mak2002 and @CalebJohn!

mak2002 and others added 6 commits July 11, 2022 23:58

initial commit

0c95341

add tests and plugin files for testing

7e0aeae

Merge branch 'laurent22:dev' into bundle-default-plugins-1

4e04b9e

small fix

b926ee6

update building script and refactor tests

e6ded17

add message when bundling plugins

68a611f

CalebJohn reviewed Jul 20, 2022

View reviewed changes

bundle plugins only when publishing desktop app

427da27

laurent22 reviewed Jul 28, 2022

View reviewed changes

mak2002 and others added 3 commits August 4, 2022 19:01

update naming | remove tsc command

a327f8a

add new mock file

4cf0d00

Merge branch 'laurent22:dev' into bundle-default-plugins-1

267e5ed

laurent22 reviewed Aug 8, 2022

View reviewed changes

CalebJohn reviewed Aug 9, 2022

View reviewed changes

mak2002 and others added 5 commits August 15, 2022 23:51

update tests with mock fetch

3d4bfd2

Merge branch 'bundle-default-plugins-1' of https://github.com/mak2002…

cc59c90

…/joplin into bundle-default-plugins-1

restore dir after running bundling plugins script

725e003

update function for reading local plugins versions

e87edf5

Merge branch 'dev' into bundle-default-plugins-1

25f636d

laurent22 reviewed Aug 27, 2022

View reviewed changes

mak2002 added 4 commits August 28, 2022 19:02

add types

c77c180

Merge branch 'bundle-default-plugins-1' of https://github.com/mak2002…

8c692f8

…/joplin into bundle-default-plugins-1

remove a new line

e46c305

update mock value for NPM response

ddf69ae

laurent22 reviewed Aug 28, 2022

View reviewed changes

update error messages

15a3dae

laurent22 merged commit 3942029 into laurent22:dev Sep 1, 2022

mak2002 mentioned this pull request Mar 5, 2023

Automate Cytoscape.js release process using GitHub Actions nrnb/GoogleSummerOfCode#224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Desktop: Bundle default plugins with desktop application #6679

Desktop: Bundle default plugins with desktop application #6679

mak2002 commented Jul 19, 2022

CalebJohn Jul 20, 2022

mak2002 Jul 20, 2022

laurent22 Jul 28, 2022

laurent22 Jul 28, 2022

laurent22 Aug 8, 2022

mak2002 Aug 8, 2022

laurent22 Jul 28, 2022

laurent22 Jul 28, 2022

laurent22 Jul 28, 2022

mak2002 Jul 29, 2022

laurent22 Aug 8, 2022

mak2002 Aug 8, 2022

laurent22 Aug 8, 2022

laurent22 Aug 8, 2022

laurent22 Aug 8, 2022

laurent22 Aug 8, 2022

laurent22 Aug 8, 2022

laurent22 Aug 8, 2022

laurent22 Aug 8, 2022

CalebJohn Aug 9, 2022

mak2002 Aug 12, 2022

laurent22 left a comment

laurent22 Aug 27, 2022

mak2002 Aug 28, 2022

laurent22 Aug 27, 2022

mak2002 Aug 28, 2022

laurent22 Aug 28, 2022

mak2002 Aug 28, 2022

laurent22 Aug 28, 2022

mak2002 Aug 28, 2022

laurent22 commented Sep 1, 2022

Desktop: Bundle default plugins with desktop application #6679

Desktop: Bundle default plugins with desktop application #6679

Conversation

mak2002 commented Jul 19, 2022

Summary

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

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

Choose a reason for hiding this comment

laurent22 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

laurent22 commented Sep 1, 2022