Show plugin (name and version) loaded #1826
Conversation
chabou
added
the
🎨 Type: Enhancement
app/index.js
Outdated
| @@ -395,7 +395,8 @@ app.on('ready', () => installDevExtensions(isDev).then(() => { | |||
| createWindow, | |||
| updatePlugins: () => { | |||
| plugins.updatePlugins({force: true}); | |||
| } | |||
| }, | |||
| getPlugins: plugins.getModules | |||
There was a problem hiding this comment.
Why do you rename it from getModules to getPlugins 🤔 I think it's better to have the same name so it's easier to read the code :)
There was a problem hiding this comment.
Because everywhere in plugin file, modules are mentioned. And plugins.getPlugins seems weird to me.
But you're right, it's really better to not mention module in exports.
There was a problem hiding this comment.
getLoadedPlugins will be a better name and it will export only plugin name and version. Not the entire module.
app/menu.js
Outdated
| module.exports = ({createWindow, updatePlugins}) => { | ||
| module.exports = ({createWindow, updatePlugins, getPlugins}) => { | ||
| const showAbout = () => { | ||
| const pluginList = getPlugins().length === 0 ? |
There was a problem hiding this comment.
save the plugins in a variable
const plugins = getPlugins()
at the top of this function, that way you avoid calling it multiple times 👍
There was a problem hiding this comment.
oups... Totally agreed
lib/utils/plugins.js
Outdated
| @@ -69,6 +69,7 @@ const clearModulesCache = () => { | |||
| }; | |||
|
|
|||
| const getPluginName = path => window.require('path').basename(path); | |||
| const getPluginVersion = path => window.require(window.require('path').resolve(path, 'package.json')).version; | |||
There was a problem hiding this comment.
This should probably be in a try/catch, just to be safe 😄 maybe if a plugin is not installed yet etc?
There was a problem hiding this comment.
Also I'd import path to a variable
const path = window.require('path')
not really necessary since require caches it but looks cleaner
There was a problem hiding this comment.
You're right, a local plugin withoutpackage.json file can be used.
There was a problem hiding this comment.
You mean like that https://github.com/zeit/hyper/blob/1.3.3/app/plugins.js#L129-L142
😰
😄
|
I was certainly tired to make a PR of such poor quality. 😰 |
| @@ -307,6 +311,13 @@ function requirePlugins() { | |||
|
|
|||
| // populate the name for internal errors here | |||
| mod._name = basename(path); | |||
| try { | |||
| // eslint-disable-next-line import/no-dynamic-require | |||
| mod._version = require(resolve(path, 'package.json')).version; | |||
There was a problem hiding this comment.
this will be sync and block other stuff, better to do something like:
const getVersion = (path) => new Promise((resolve, reject) => {
fs.readFile(resolve(path, 'package.json'), (error, result) => {
if (error) return reject(error)
try {
const { version } = JSON.parse(result.toString())
return resolve(version)
} catch (error) {
return reject(error)
}
})
})or just use https://github.com/sindresorhus/load-json-file 😄
There was a problem hiding this comment.
maybe not an issue since it's (maybe?) cached, and looking at your screenshot it doesn't even take 10ms
There was a problem hiding this comment.
yes I think it is not an issue, I measure it in renderer with 3 plugins without requiring package.json: 8ms.
With requiring: 12ms.
I don't want to add asynchronous complexity here and in the renderer for such a little gain.
| const getPluginVersion = path => { | ||
| let version = null; | ||
| try { | ||
| version = window.require(pathModule.resolve(path, 'package.json')).version; |
There was a problem hiding this comment.
|
Great work @chabou ❤️ |
|
rebased and Copyright added Ready to merge |
|
Is it OK'd by the @zeit team? |
|
It seems impossible to easily add link to this Electron MessageBox. |
Currently, there is no way to know which plugin is loaded and its version.
I begun to add a
console.logto my plugins but I realized it can be a useful information and should be a Hyper feature.This PR add these informations:
To show these information in about dialog, I needed to use a custom Dialog on MacOS like on Windows/Linux. The only downside is that I can't show build number like currently:
But now, MacOS About dialog is consistant with other OS.
@rauchg: I don't know if you want to add Zeit Copyright (but it is not currently shown on Windows/Linux).