-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Plugin requirement hoc #260
Conversation
…o content-type-builder-plugin
…on to ProductionBlocker component
@@ -10,5 +10,9 @@ | |||
"app.components.LeftMenuLinkContainer.noPluginsInstalled": "Aucun plugin installé", | |||
"app.components.LeftMenuLinkContainer.plugins": "Plugins", | |||
"app.components.NotFoundPage.description": "Page introuvable", | |||
"app.components.NotFoundPage.back": "Retourner à la page d'accueil" | |||
"app.components.NotFoundPage.back": "Retourner à la page d'accueil", | |||
"components.AutoReloadBlocker.header": "L' autoReload doit être activé pour ce plugin.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
L'autoReload doit être activé pour ce plugin.
@@ -1,6 +1,6 @@ | |||
{ | |||
"languages": ["en", "fr"], | |||
"plugins": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this because we don't it anymore.
|
||
import configureStore from './store'; | ||
import { translationMessages } from './i18n'; | ||
|
||
const tryRequire = (bootstrap = false) => { | ||
try { | ||
const path = 'bootstrap'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need this variable.
</p> | ||
<div className={styles.ide}> | ||
<p>./config/environments/development/server.json</p> | ||
<div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use the <pre></pre>
instead.
@@ -0,0 +1,3 @@ | |||
export const shouldRenderCompo = (plugin) => new Promise((resolve) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const shouldRenderCompo = (plugin) => Promise.resolve(plugin);
.then(menu => { | ||
plugin.leftMenuSections = menu; | ||
|
||
resole(plugin); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use resolve
instead of resole
.
@@ -1,6 +1,6 @@ | |||
{ | |||
"languages": ["en", "fr"], | |||
"plugins": { | |||
"ports": [3000] | |||
"ports": [3000, 5000, 8000] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This config should be removed.
|
||
import configureStore from './store'; | ||
import { translationMessages } from './i18n'; | ||
|
||
const tryRequire = (bootstrap = false) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argument should be the name of the file: fileName
. So you can call the function in a more generic way: tryRequire('bootstrap');
and tryRequire('pluginRequirements');
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried and it didn't work that's why there was a const path = 'bootstrap';
that I forgot to delete
switch (true) { | ||
// Execute bootstrap function and check if plugin can be rendered | ||
case isFunction(plugin.bootstrap) && isFunction(plugin.pluginRequirements): | ||
plugin.pluginRequirements(plugin) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid nested promises:
plugin.pluginRequirements(plugin)
.then(plugin => {
return plugin.bootstrap(plugin);
})
.then(plugin => {
store.dispatch(pluginLoaded(plugin));
});
try { | ||
const path = 'bootstrap'; | ||
const config = bootstrap ? require('bootstrap').bootstrap : require('pluginRequirements').shouldRenderCompo; | ||
return config; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a dynamic return: return require(fileName);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't do that since passing a fileName parameter didn't work
import { generateMenu } from 'containers/App/sagas'; | ||
|
||
// This method is executed before the load of the plugin | ||
export const bootstrap = (plugin) => new Promise((resole, reject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Export as default function.
@@ -0,0 +1,3 @@ | |||
export const shouldRenderCompo = (plugin) => new Promise((resolve) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Export as default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name of the file should be requirements.js
instead of pluginRequirements.js
in order to be consistent with bootstrap.js
.
<div> | ||
|
||
<h4> | ||
<FormattedMessage id="components.AutoReloadBlocker.header" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't better to create a single, but dynamic Blocker component, that support props like title
, description
and content
(which could even be a component defined in the plugin)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plugin is in charge of telling which component to render if we prevent it from being displayed. In this case passing such logic is a bit more complex and since we're rendering stateless component instead of the plugin itself I thought that having two different blockers or whatever other component provided by the plugin is a great deal.
LGTM |
Add
pluginRequirements.js
method that prevents a plugin from being rendered if some conditions aren't met.