Remove postinstall script

[yuchi]

Se the conversation that lead to this on this commit.

Installing dependencies in a postinstall script is a bad practice for the following reasons:

• you depend on the quality of the environment (having npm in the PATH);
• you install a dynamic list of packages inside a dependency, which makes debugging less clear;
• you make upgrading difficult («what if I want to upgrade from 6.2 to 7.0?»);
• you create extraneous installed packages, and that’s the most “offensive” issue.

I therefore propose to:

1. Have those dependencies installed on the theme level, and saved in the theme’s package.json.
2. Have those automatically installed either (non mutually exclusive)
   • by generator-liferay-theme
   • by presets (e.g. liferay-theme-preset-6.2)

[robframpton]

Hey @yuchi

I definitely agree that the current setup is not ideal, let me explain how we ended up with the current implementation.

The reason we have this post install script is that we currently support 6.2 and 7.0 which have different theme dependencies. We also support different Sass compilers (Ruby Sass or libSass), which are fairly large dependencies so we didn't want to install them unless absolutely necessary.

So after all of the shared dependencies are installed (everything listed in package.json), the post install script sniffs out what version of Liferay the current theme is intended for, and what Sass compiler is being used, and then dynamically installs the remaining dependencies. So currently it's not just dynamically installing things based on Liferay version, but other properties as well (which complicates things).

In regards to your concerns.

depend on the quality of the environment (having npm in the PATH);

This is true, for the meantime I wrote a workaround that checks to see if the local npm module is available (require('npm')) and invokes the install command directly rather creating a child process. This module is available the way we are installing node in Portal. It will then fallback to creating the child process if the npm module cannot be found. We can use this until a better solution is implemented.

you make upgrading difficult («what if I want to upgrade from 6.2 to 7.0?»);

The upgrade task that is a part of the build tools will actually invoke this script and install the correct dependencies based on the new version listed in the package.json. So upgrading from 6.2 to 7.0 is not difficult at all.

you install a dynamic list of packages inside a dependency, which makes debugging less clear; / you create extraneous installed packages, and that’s the most “offensive” issue.

One idea is to list all of these dependencies in devDependencies in package.json. They are in fact required to run the tests anyways, so it's not inaccurate to claim them as devDependencies. It would also resolve the extraneous errors when running npm commands.

With all that said, I still agree with you that installing additional packages in the post install script isn't ideal. After thinking about it, this is what I'm planning to implement.

• Creating presets as you said for both 6.2 and 7.0 that include all of the version specific dependencies which will be added to both the theme's package.json (by the yeoman generator) and the devDependencies of liferay-theme-tasks for testing.
• Removing baked in support for Ruby Sass for 7.0 themes (and therefore removing the need for dynamic Sass dependencies) and creating a hook that can be implemented in the theme if developers need to use Ruby Sass for 7.0.

Thanks @yuchi for the feedback.

[yuchi]

Thank you for the amazing work and for listening to my complaints :)

There's just a single last piece of info that I'd like to share: using presets is very easy on npm v3 because dependencies are flatted out, but can break hideously when the dependency graph has collisions.

The preset of dependencies will be the one to be required, and it will give the list of dependencies it ‘bundles’. You'll then need to require them relatively to the preset using something along the lines of

require(require.resolve(dep, require.resolve(preset))

This is doable with the resolve package (good) or by exporting the module itself inside the preset (bad).

[yuchi]

Here is the docs about that bad thingy: https://nodejs.org/api/modules.html#modules_module_require_id

😬

[yuchi]

Looks like we can close it now!

[robframpton]

Still waiting for a small change to get merged into liferay-portal, once that change is new versions of liferay-theme-tasks and generator-liferay-theme will be published with the changes we've discussed.