Including the presets and plugins module path into `config.metadata` #43

DanielSanudo · 2019-08-05T15:22:08Z

Summary

Including the presets and plugins module path into config.metadata.presets and config.metadata.plugins.

This will allow us to access to the module path from the plugin lifecycles. Immediate use cases will be for metadata and docs lifecycles.

Changelog

Test Plan

Unit tests included for the different cases.

packages/gasket-plugin-engine/lib/engine.js

SivanMehta

Hit the wrong radio 🤦‍♂

should have been a comment, not an approval

kinetifex · 2019-08-05T18:31:07Z

packages/gasket-resolve/resolver.js

      fullName => `Plugin ${fullName} could not be resolved. Make sure it is installed.`);
  }

+  pluginFullName(name) {
+    return `@gasket/${name}-plugin`;


This only works for short-named gasket plugins. This is not going to work for user plugins (i.e. @my/fancy-gasket-plugin) or if the user specified the full name in the config (i.e. @gasket/express-plugin).

Instead, let's centralize these by using the pluginIdentifier helper from:
https://github.com/godaddy/gasket/blob/master/packages/gasket-cli/src/scaffold/package-identifier.js

It would probably be best to move package-identifier.js and tests from the cli to the resolve package, with the identifiers exposed for the cli to also utilize:

const { pluginIdentifier } = require(`@gasket/resolve`);

kinetifex · 2019-08-05T18:33:13Z

packages/gasket-resolve/resolver.js

@@ -32,7 +36,7 @@ module.exports = class Resolver {

    return this.resolveShorthandModule(
      name,
-      shortName => `@gasket/${shortName}-preset`,
+      this.presetFullName,


See previous comment, and use presetIdentifier here instead.

kinetifex · 2019-08-05T18:42:49Z

packages/gasket-plugin-engine/lib/engine.js

+      if (pluginName.indexOf('/') !== -1) {
+        const relativePath = path.relative(rootPath, pluginName);
+        const pluginKey = path.basename(pluginName).replace('-plugin', '');
+        this.config.metadata.plugins[pluginKey] = { modulePath: `./${relativePath}` };


Need to avoid using / directly in your paths. This will likely cause issues on Windows machines.

Swaagie

Nice work, no additional comments, on green ✅ from all reviewers

Swaagie · 2019-08-06T15:13:21Z

packages/gasket-plugin-engine/lib/engine.js

+   * Saves into the gasket config the module path of the presets
+   *
+   * @private
+   * @param {Array} presets Array of presets


Nitpick: this order is usually inverted, e.g. params first and then scope.

DanielSanudo · 2019-08-07T14:35:32Z

@SivanMehta @kinetifex @Swaagie Related to building the paths, this is the format that was defined in the docs, https://github.secureserver.net/UX/uxp-pdrs/blob/master/gasket/docs-command.md#gathering-data.

I ended up getting

modulePath: '/node_modules/@gasket/plugin-fastify'

instead of ( see the dot at the start of the path )

modulePath: './node_modules/@gasket/plugin-fastify'

by using the following code. Is there another way of getting what was defined in the docs?

relativePath = path.join('//', relativePath);

kinetifex · 2019-08-07T15:08:02Z

Oh, I see why you were trying to do that now. It's actually sufficient to just have relative path be:

modulePath: 'node_modules/@gasket/plugin-fastify'

and not bother trying to add the extra ./

kinetifex

I honestly don't know the resolve or plugin-engine code base too well.
I'd definitely recommend a review from @indexzero here, though I left some comments regarding some code organization that make help keeps things cleaner.

kinetifex · 2019-08-07T19:09:15Z

packages/gasket-resolve/resolver.js

@@ -13,10 +14,14 @@ module.exports = class Resolver {

    return this.resolveShorthandModule(
      name,
-      shortName => `@gasket/${shortName}-plugin`,
+      this.pluginFullName,


We don't need this pluginFullName static method, instead just use pluginIdentifier directly here.

Suggested change

this.pluginFullName,

shortName => pluginIdentifier(shortName).fullName,

kinetifex · 2019-08-07T19:10:16Z

packages/gasket-resolve/resolver.js

@@ -32,7 +37,7 @@ module.exports = class Resolver {

    return this.resolveShorthandModule(
      name,
-      shortName => `@gasket/${shortName}-preset`,
+      this.presetFullName,


We don't need this presetFullName static method, instead just use presetIdentifier directly here.

Suggested change

this.presetFullName,

shortName => presetIdentifier(shortName).fullName,

kinetifex · 2019-08-07T19:12:01Z

packages/gasket-resolve/test/resolver.test.js

+
+  it('pluginFullName returns expected full name', () => {
+    const resolver = new Resolver();
+    const result = resolver.pluginFullName('pluginName');


Drop these fullName static methods and tests from Resolver, since they are effectively redundant to the identifier functions.

kinetifex · 2019-08-07T21:28:27Z

packages/gasket-plugin-engine/lib/engine.js

+   * @private
+   * @param {Array} plugins Array of plugins
+   */
+  _registerPluginsModulePath(plugins) {


It feels like a lot of this code should be owned by the Resolver, and is related to:
https://github.com/godaddy/gasket/blob/master/packages/gasket-resolve/resolver.js#L52-L55

Maybe add a related resolver.tryResolve like:

tryResolve(moduleName) { const options = this.resolveFrom ? { paths: [this.resolveFrom] } : {}; // may want to wrap this in a try/catch return require.resolve(moduleName, options); }

That way, you can utilize the resolveFrom option, instead of deriving your own rootPath.

See the require.resolve docs.

kinetifex · 2019-08-07T21:30:13Z

packages/gasket-plugin-engine/lib/engine.js

+   * @returns {Path} root path of the app
+   */
+  _rootPath() {
+    return path.resolve(__dirname).split('/node_modules')[0];


The root path is already provided by the cli when instantiating the PluginEngine. See: https://github.com/godaddy/gasket/blob/master/packages/gasket-cli/src/config/loader.js#L11

It would be good to clear this up in the PluginEngine API, and add a safety check with:

class PluginEngine { constructor(config, { resolveFrom } = {}) { this.config = config || {}; + this.config.root = this.config.root || process.cwd()

kinetifex · 2019-08-07T21:31:21Z

packages/gasket-plugin-engine/lib/engine.js

+   * @param {String} module module
+   * @returns {Path} path of the module
+   */
+  _resolveModulePath(module) {


too many consoles.logs!

But either way, probably would be better owned by the Resolver (see other comment)

How this ended up here xd, I will remove the logs.

kinetifex · 2019-08-12T15:31:40Z

packages/gasket-plugin-engine/lib/engine.js

+        var pluginNameKey;
+        // Plugins that are defined locally to the app
+        if (pluginName.indexOf(this.config.root) !== -1) {
+          pluginNameKey = path.basename(pluginName).replace('-plugin', '');


Do we need really this fixup to remove -plugin? Seems like that could give confusing results, being a short name, which is reserved for @gasket/<name>-plugin formatted plugins.

In fact, it would probably be best to include plugins/ in this key name, to delineate an app plugin, from a node_module plugin.

So, for example, if a user has these plugins:

example — resolved short name for @gasket/example-plugin

example-plugin

plugins/example-plugin — avoids collision with example-plugin

This code is only applied to the local plugins that are defined under plugin/something-plugin, it will not affect to any @gasket/<name>-plugin. What I am doing is getting something-pluginand removing -plugin from it to use it as the key.
Metadata will appear as this:

metadata: { plugins: { pluginA: 'plugins/pluginA-plugin' } }

If you prefer to have something else as key I can work on that.

kinetifex · 2019-08-12T15:37:27Z

packages/gasket-resolve/resolver.js

+   * @returns {Path} relative path of the preset
+   * @public
+   */
+  tryResolvePresetRelativePath(presetName, appRootPath) {


tryResolvePresetRelativePath and tryResolvePluginRelativePath are effectively the same code. Why not just have a single tryResolveRelative(pkgName) then the consumer passes in the package name they want. That way, this could resolve for non-preset/plugin packages, too.

They are not exactly the same, I need to use presetIdentifier or pluginIdentifier depending on which one and also I need to handle the case of a local plugin before calling pluginIdentifier.

kinetifex · 2019-08-12T15:42:26Z

packages/gasket-resolve/resolver.js

+   * Returns the relative path of the preset
+   *
+   * @param {String} presetName name of the preset
+   * @param {String} appRootPath root path of the app


How about just make root an option when instantiating the Resolver? Then the consumer doesn't have to keep track and pass it to this method.

constructor({ resolveFrom, resolve, root = process.cwd() } = {}) {

SivanMehta previously approved these changes Aug 5, 2019

View reviewed changes

packages/gasket-plugin-engine/lib/engine.js Outdated Show resolved Hide resolved

packages/gasket-plugin-engine/lib/engine.js Outdated Show resolved Hide resolved

SivanMehta reviewed Aug 5, 2019

View reviewed changes

kinetifex requested changes Aug 5, 2019

View reviewed changes

Swaagie approved these changes Aug 6, 2019

View reviewed changes

Swaagie reviewed Aug 6, 2019

View reviewed changes

kinetifex requested changes Aug 7, 2019

View reviewed changes

kinetifex requested changes Aug 12, 2019

View reviewed changes

kinetifex approved these changes Aug 12, 2019

View reviewed changes

kinetifex requested a review from indexzero August 12, 2019 17:01

DanielSanudo added 15 commits August 13, 2019 14:19

Adding plugin/preset module paths to config

4147b69

Cleaning @gasket/plugin-engine/engine.js

31b1e4f

Reverting one change

9aa2016

Linting

602af6a

Starting to add tests

0b8b235

Adding tests for resolver

0795686

Fixing tests

8422a9c

Fixing tests

9c64893

Updating CHANGELOG.md

a5b3e94

Moving from to

0151543

Usinng and for resolving full names

df751c8

Adding support to custom plugins

d694db9

Moving code into @gasket/resolve and using

f5ed2a8

Adding root path as a constructor parameter for Resolver

04f6f59

Updating CHANGELOG.md

c002a8a

DanielSanudo force-pushed the register_module_paths branch from 07cec4d to c002a8a Compare August 13, 2019 13:21

DanielSanudo merged commit cbe42ca into master Aug 13, 2019

DanielSanudo deleted the register_module_paths branch August 13, 2019 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Including the presets and plugins module path into `config.metadata` #43

Including the presets and plugins module path into `config.metadata` #43

DanielSanudo commented Aug 5, 2019

SivanMehta left a comment

kinetifex Aug 5, 2019

kinetifex Aug 5, 2019

kinetifex Aug 5, 2019

Swaagie left a comment

Swaagie Aug 6, 2019

DanielSanudo commented Aug 7, 2019 •

edited

Loading

kinetifex commented Aug 7, 2019

kinetifex left a comment

kinetifex Aug 7, 2019

kinetifex Aug 7, 2019

kinetifex Aug 7, 2019

kinetifex Aug 7, 2019

kinetifex Aug 7, 2019

kinetifex Aug 7, 2019

DanielSanudo Aug 8, 2019

kinetifex Aug 12, 2019

DanielSanudo Aug 12, 2019 •

edited

Loading

DanielSanudo Aug 12, 2019

kinetifex Aug 12, 2019

DanielSanudo Aug 12, 2019

kinetifex Aug 12, 2019

	this.pluginFullName,
	shortName => pluginIdentifier(shortName).fullName,

	this.presetFullName,
	shortName => presetIdentifier(shortName).fullName,

Including the presets and plugins module path into config.metadata #43

Including the presets and plugins module path into config.metadata #43

Conversation

DanielSanudo commented Aug 5, 2019

Summary

Changelog

Test Plan

SivanMehta 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

Swaagie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielSanudo commented Aug 7, 2019 • edited Loading

kinetifex commented Aug 7, 2019

kinetifex 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

DanielSanudo Aug 12, 2019 • 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

Including the presets and plugins module path into `config.metadata` #43

Including the presets and plugins module path into `config.metadata` #43

DanielSanudo commented Aug 7, 2019 •

edited

Loading

DanielSanudo Aug 12, 2019 •

edited

Loading