Introduce NPM packages as External Plugins

[f1ames]

So tho whole change required adding package.json (which wasn't there :scream: ). We could do this without it (e.g. running npm i name --no-save --something-something) but I think it will be useful in the future.

I'm not sure about package version, if ti should be aligned with CKEditor 4 version or just something like 1.0.0 or 0.0.1, WDYT?

I also added private: true which prevents from publishing this as package by mistake.

Now, one more thing is why npm i is a part of build process? We have some other tools like docs, online builder, etc. which uses presets in a way that they run git submodules update and then build. Adding separate step which is not a part of build process will require changes in all related tools (so kind of a breaking change). Adding npm i as a build step makes this change backwards compatible.

Another thing is that, this change assumes that build process is run in an env where npm command is available. It was partially true before (for -t flag) so I hope it won't cause any issues.

[f1ames]

@jacekbogdanski I'm generally fine with your proposal from https://github.com/ckeditor/ckeditor4-presets/pull/40#pullrequestreview-479499510.

The only thing which bothers me is having plugin.txt file which basically does the same thing what dependencies in package.json. And the general issue is that the more custom solution we choose the harder it will be to follow for others. Going with package.json and npm i is just a standard way in JS env and most devs will grasp what's going on in a matter of seconds. Going with custom files to store deps and using (not that obvious) npm pack adds some cognitive overload for everyone who tries to understand what's going on.

We can make this changes more general also with package.json and make copying stuff from node_modules more relaxed (copy all, whitelist, blacklist, anything).

But adding anything more will require changing this script again or bloating it with additional plugins. Also, changing bash scripts is not favorite thing to do for most devs.

Well, I don't think it would happen that often. It's the first time since we use presets so not sure if that's such an issue. And going with custom solution instead of standard one just to save someone editing bash script - not sure if it's worth it.

WDYT?

[jacekbogdanski]

We are using a custom build script with some Java lib help. I'm not sure if using package.json gives any value at this point (if we are talking about simplicity), especially that's it's used in a totally different purpose than normally (plugins are moved). So, referring to

the more custom solution we choose the harder it will be to follow for others

Everything here is custom, I think that adding something more common on top of it doesn't really help, more likely introduces more confusion because common solution is used in an uncommon way.

But I agree that we are not really changing anything in presets very often, so it also doesn't make sense to create a very generic solution for something which is unlikely to change to avoid over-engineering. Still, using package.json for this problem seems to me more complicated than downloading list of plugins.

[jacekbogdanski]

One of the nicer things of package.json is that it's much easier to keep track of the dependency version. Not sure if it's not a killer feature here in favor of package.json. The question is if we really need it during the testing phase - more likely we want to always test the latest package.

[f1ames]

@jacekbogdanski So maybe let's go with plugin names array hardcoded in build.sh script (since it won't change often) and with npm pack + tar. This way we don't have custom plugin.txt file or package.json and it's just as simple as it should be. WDYT?

:point_up: I wrote it before your last comment about:

One of the nicer things of package.json is that it's much easier to keep track of the dependency version. Not sure if it's not a killer feature here in favor of package.json.

You can still go with npm pack name@version so the pluign list will also somehow track plugins versions.

[jacekbogdanski]

You can still go with npm pack name@version so the pluign list will also somehow track plugins versions.

That's right :+1:

@jacekbogdanski So maybe let's go with plugin names array hardcoded in build.sh script (since it won't change often) and with npm pack + tar. This way we don't have custom plugin.txt file or package.json and it's just as simple as it should be. WDYT?

Works for me. I wasn't totally convinced to the separate file as it creates a more complicated solution than we need, so a simple list inside build.sh should work.

[Comandeer]

Personally I'm for doing it with package.json. The whole copying thing can be integrated via postinstall npm script – this way it will be much easier to grasp what's happening with dependencies.

[jacekbogdanski]

I'm not really sure if adding even more scripts to the party is a good idea.

[f1ames]

Well, @Comandeer you haven't made it easier. The more I look into this the more changes in the build script I would like to add to make it a proper solution... :thinking:

I'm slightly in favor of package.json but again it adds some complexity on top of just hardcoding in build.sh. Post-install still needs to know which packages to move. I will rework the current proposal to see how it looks with both approaches maybe.

[f1ames]

Ok, so assuming we will have have hardcoded plugins list it can look like:

externalPlugins=('ckeditor4-plugin-exportpdf@1.0.0')

Then we can iterate through it and do npm pack $plugin.

Since it will be saved to ckeditor4-plugin-exportpdf-1.0.0.tgz we need to do some replacing (@ to -) to have correct name for extraction (what if package has @ in the name like https://www.npmjs.com/package/@wiris/mathtype-ckeditor4?).

Then we can got with tar - xzf but we also need output folder name (which probably should be stored in another array or associative array).

I have the feeling we are reinventing a wheel of simple npm i and also it adds a little bit of bash magic which is not super fun to maintain.

This makes me think that going with package.json solves some low-level issues out-of-the-box and is easier to maintain.

That being said, I would stick with package.json and basically with the current proposal.