Add a new build step : "Asset provisioning"

[prinzdezibel]

The idea is that a plugin can hold its own assets in a folder called /private. During build of the application, the content of this folder is copied to /private/plugins/. This seems to me like a sensible way (the only?) way to actually bundle the assets of the plugin for usage through Meteor's Assets API.

[prinzdezibel]

@jshimko Please see this accompanying PR in reaction core: https://github.com/reactioncommerce/reaction/pull/3335/files

[prinzdezibel]

@zenweasel Maybe it makes even sense to have such a build step for public resources as well? Because right now, AFAIK, no plugin can hold public resources in its /public directory.

[brent-hoover]

@jshimko Could you please review and approve if appropriate?

[prinzdezibel]

@jshimko Actually I've talked to @zenweasel today and we came up with the idea of having the asset provisioning step to work a bit different than what you're seeing within this PR: Instead of using the plugin's directory name it would be better to use the name of the plugin itself as found in register.js.

This would allow the user to clone the plugin with any name he likes into APP_ROOT/imports/plugins/custom/, e.g. /imports/plugins/custom/my-crazy-plugin-name. Doing so with the current implementation, the assets would be copied (via the asset provisioning step) to "APP_ROOT/private/plugins/my-crazy-plugin-name" which in turn would require to adapt code in the plugin itself.

I'm going to update this very PR soon. Just wanted to let you know before you are spending a lot of time.

[prinzdezibel]

@jshimko, @zenweasel Pushed proposed changes.

[jshimko]

Ok, a few things...

I see several linting issues in this PR. Please make sure you're using ESLint in your editor so your code formatting matches the project's config. For example, I see use of both single and double quotes and some inconsistent spacing with curly braces. ESLint should be showing you warnings for all of that (let me know if it's not).

Also, any synchronous methods (e.g. fs.copySync(dir, targetDir)) should always be wrapped in a try/catch block with proper error handling. Not doing so can result in confusing Node errors if that method fails for some reason.

And I think we need to reconsider the idea of using the register.js package name as a directory name because that could get a little complicated. For one, that's not actually exported anywhere. It's only passed into Reaction.registerPackage() (which I now see you just pushed some changes to parse that raw code out of the file, but I'm not sure that's the ideal solution). Secondly, a register.js is not currently required for a plugin to load and work and I'm thinking it shouldn't have to be a requirement just to load assets. Files are generally loaded by directory path, not a generated directory path based on some config in an optional file. Aside from adding a new config file requirement, it just seems like that inconsistency of paths could be confusing for some people.

My thinking on this was that it would probably be a lot simpler and clearer for users if the directory name mattered like it does in any other place in the app where you'd import a file. I also think core, custom, and included need to be in that output path because we're going to want to use this for any internal Reaction plugin as well. Throwing everything in /private/plugins runs the risk of name collisions or possible confusion about where it was copied from. That said, I think we should set the destination subdirectory based on where the plugin is in the imports directory. So...

/private/plugins/core/<plugin_dir_name>
/private/plugins/custom/<plugin_dir_name>
/private/plugins/included/<plugin_dir_name>

/public/plugins/core/<plugin_dir_name>
/public/plugins/custom/<plugin_dir_name>
/public/plugins/included/<plugin_dir_name>

Then all users need to know for loading an asset in their plugin would be:

// private stuff on the server
Assets.getText("/plugins/custom/<plugin_dir_name>/whatever.json");

or

<!-- public stuff on the client -->
<img src="/plugins/custom/<plugin_dir_name>/whatever.jpg">

I also think we need to have a cleanup process that clears out the generated assets on every new build of the app (each time the CLI starts). That's what happens with both the javascript and the styles loaders. That way when you delete something from a plugin, you can be sure you don't end up with stale files in the copied location that no longer exist in the plugin. You can accomplish that with:

import rimraf from 'rimraf';

rimraf(`${appRoot}/private/plugins/*`);
rimraf(`${appRoot}/public/plugins/*`);

So, I'm obviously not the final word on all of this stuff, but I think these are all valid points that are worth discussing a little further before we commit to something that'll be hard to change later. Especially with some of the people on the team that have written a lot of plugins. Thoughts @spencern, @mikemurray, or @kieckhafer?

[prinzdezibel]

@jshimko thanks for your input. Your thoughts on the following points are clear to me and a no-brainer:

• Linting
• try/catch
• cleanup step
• namespacing (core/included/custom)

Where it gets interesting is the paragraph 4

My thinking on this was that it would probably be a lot simpler and clearer for users

which is exactly how the first PR (before my changes today) worked. But it has the drawback that once you're going to change the directory name of the plugin you will end up with a non-working plugin, because within the plugin there's a lot of code like

Assets.getText("/plugins/custom//whatever.json");

.hero { background-image: url("/plugins//landing-hero.jpg"); }

const filepath = plugins/<plugin_dir_name>/images/${productId}.jpg; const binary = Assets.getBinary(filepath);

which is why we came up with the idea in paragraph 3

And I think we need to reconsider the idea of using the register.js package name as a directory name

Saying that, your concerns are of course completely valid. The question is how we can be flexible with our directory names without the need to adapt plugin code.

[jshimko]

Well honestly, if I change a directory name, I'd expect it to break import paths for files I'm importing from there, so I'm not sure that's going to surprise too many people. I mean, that's literally the case for 100% of files in the app at this point. If you rename it, you break any import of it.

And requiring a register.js to import some of the plugin files when others (css/js) are imported automatically just by being in the right location is potentially a little confusing as well. Combined with the auto-generating folder paths from a name in a register.js, I don't personally find that to be a very user-friendly scenario. Nor do I see many advantages to that (other than being able to rename my plugin directory, but how often does that really happen?)

Consider the following example. If I have a logo image in my plugin at:

/imports/plugins/custom/my-plugin/public/logo.png

but my register.js names the plugin something different like my-default-theme... now my asset imports look like they're coming from a different plugin.

.logo {
  background-image: url("/plugins/custom/my-default-theme/logo.png");
}

I find that confusing and I don't see any advantages to it other than being able to rename a plugin directory without changing asset imports. But if you were importing styles or js from that same plugin, you'd have to update those imports. So at the very least, it's not consistent with any other type of static file import.

I don't know. I'm not the one that has to use this stuff most of the time, but I definitely don't prefer that scenario. I can already imagine the support questions popping up repeatedly from that.

But that's just my $0.02 :)

[prinzdezibel]

Not sure I've explained my intentions clearly enough. I'm not so much concerned about people changing the directory name by intention without having to refactor code, but the mere fact that one would be forced to install a arbitrary plugin exactly with the directory name the plugin author has chosen for him.

$ cd <app_root>/imports/plugins/custom
// The following will work fine!
$ git clone https://github.com/reactioncommerce/reaction-swag-shop.git 
// as opposed to this: 
$ git clone https://github.com/reactioncommerce/reaction-swag-shop.git custom-dir-name 
// will not work at all :(

[prinzdezibel]

I like your analogy regarding "imports". But from my point of view, "private assets" and "public resources" don't necessarily map directly to resources on the file system. For me it's completely ok to see them as URIs without direct correlation to the underlying file system. The same way a webserver does not necessarily expose the file hierarchy directly (unless one is using PHP :-)

[prinzdezibel]

@jshimko I agree to all of your points, the one thing that I don't like is to have the plugin author to impose the path in which it has to be installed.

I've been thinking about it again and would like to propose another way of handling it. What if the asset provisioning step generates some helper functions/variables in the plugin root that can be used to obtain a path to the assets/resources?

This way the provisioning can work the way you proposed and the plugin author doesn't need to know the directory name when referring to resources.

E.g.

.hero {
 background-image: url("@{assetPath}/landing-hero.jpg");
}

  import { assetPath } from "../../pluginResources"

  const filepath = assetPath + `/images/${productId}.jpg`;
  const binary = Assets.getBinary(filepath);

Assuming that reaction-cli would have to generate those:

pluginResources.js

const assetPath = "/plugins/<plugin_dir_name>";

variables.less

@assetPath: "/plugins/<plugin_dir_name>";

If you like the idea, I can prepare a PR.

[prinzdezibel]

Edit: Perhaps using generated files that are not under source control, but imported from code are not the ideal solution either (confusing, ES-lint errors, etc.).

Probably some kind of core API function would be better suited, like

import { getPluginResourcePath } from "<somewhere in core>/pluginresources"

const pluginId = "reaction-swag-shop";  // <-- This needs some discussion. 
                                 // What does this name refer to?
                                 // - plugin's name in register.js,
                                 // - directory name,
                                 // - something entirely different?

const filepath = getPluginResourcePath(pluginId) + `/images/${productId}.jpg`;
const binary = Assets.getBinary(filepath);

[spencern]

@prinzdezibel Can you articulate in more detail exactly what problem this is trying to solve and how it resolves that problem?

I think @jshimko makes some good points as to some of the issues that people will encounter if we go this route. You wrote

the one thing that I don't like is to have the plugin author to impose the path in which it has to be installed.

It seems you're advocating for a pattern that might allow people to install a plugin into whatever directory they like. Why is this a concern? What value does that provide?

Additionally, I'm fairly certain that @aaronjudd and the @reactioncommerce/architecture team has been thinking about plugins a good bit lately.

If we moved to a fully npm-ified plugin structure (npm install reaction-payments-stripe etc) would your proposal need to change?

[prinzdezibel]

@spencern It's all about to allow the user to be in control of directory name in which the plugin will live. Brent and me think it would be very strict to tell the user: "Clone the plugin's source code into exactly this directory "/imports/plugins/custom/reaction-swag-shop", otherwise it won't work.

We both are thinking that users would prefer to choose freely the name of the directory (at least the last slug above: reaction-swag-shop). Not being able to do so will probably lead to questions from our users as well.

The problem came up when @zenweasel tried to install the plugin and used a different name for it, because he already had a plugin named "reaction-swag-shop".

NOTE: If the way we're installing plugins is done differently in future (e.g. an installer - or even reaction-cli !! - cares about where to copy the files to) , the problem may become obsolete entirely.

Re: NPM-aware-plugins Can't say much to the question, how this relates to a npm-ified plugin structure.

[prinzdezibel]

NOTE: The discussion above is related to another commit I did yesterday, not the whole PR. From what I've heard, I think all of us agree, that we need some kind of "asset provisioning step" in our build process.

[brent-hoover]

This comes from a question/problems we get in community channels almost daily, which is that you can't have any assets, specifically images in your plugin. Using "Asset provisioing" would allow people to keep images and other static assets in their plugin so that could actually follow our prescribed method of keeping their plugin separate from core.

I feel like tying plugins to specific directory names is an anti-pattern (we opted out of doing this ourselves long ago when we didn't put all included plugin in reaction-xxxx directories) but I don't think it's a hill I want to die on and I can't really articulate any strong use cases for not doing so except for the element of least surprise in that if I did rename the directory I could have a hard time seeing how that would break things.

[brent-hoover]

Also the comment about fs.copySync, isn't that a synchronous method, not an async?

[prinzdezibel]

@jshimko I've reverted the changes regarding the naming of the plugin directory for now. So the PR should satisfy your suggestions. Let's stick to your proposal until we feel we need a better solution – I'm ok with that.