decred / politeiagui

ISC License
62 stars 56 forks source link

[plugins-structure] Plugin setup #2797

Closed tiagoalvesdulce closed 2 years ago

tiagoalvesdulce commented 2 years ago
  1. I think initializerIds isn't a good name. It's not clear what this is referring to for a dev creating a brand new app for the first time. I'd rather change it to pluginInitializerRouteIds ow something like that.
  2. Would be nice to show an error (in the console or in the build process, I'd prefere the second) when a user defines a initializer without adding the plugin to the appSetup plugins array.
  3. I think this function can be improved: https://github.com/decred/politeiagui/blob/bb0ea21cca6af41de761f176ad314776998f6ecf/plugins-structure/packages/core/src/appSetup.js#L65
    • I think the filter is unnecessary if we go with the error approach. The plugin initializerId is not in initializers? Break and show an error
    • Can fire all actions in parallel. await Promise.all([...routeInitializerActions])
  4. When 2 plugins have the same initializerId the warning in the appSetup works nice. But if the dev tries to add an id to the createRoute initializerIds array and that id is not unique we should fire an error. Firing an error instead of a warning in the appSetup is an option too. It's very confusing to execute multiple actions from the same id.
  5. It's not clear how do I use a view route from a plugin in the app. For example, if I have a /tesplugin route in my testplugin plugin, how do I tell my testapp to use tesplugins's /testplugin route when we hit the /testplugin route within the app?
  6. We should document that, due to the nature of yarn monorepos, when the dev creates a plugin it should run yarn in the root before using it in an app.
victorgcramos commented 2 years ago
  1. Okay! That makes sense.
  2. Agreed. The app should break if some given plugin initializer id is not defined.
  3. The filter is required to execute the initializers from the ids array. Not all initializers will be executed for each route, so we need to filter what will be used. Makes sense? However, instead of using the array filter, i'll manually filter the existing initializers in a for of loop, and if some initializer is not defined, an error will be thrown.
  4. Nice.
  5. You can use the mergeRoutes util like we used before the plug and play setup. I'll move that util to our core router so more apps can use it.
  6. okay!