Ckeditor5: Consider switching from gulp to npm scripts to reduce bloat

[Reinmar]

We have ckeditor5 repository and ckeditor5-* sub repositories. Right now all of them use gulp.

As for ckeditor5 itself, it doesn't matter that much, but we already have some npm scripts there, so it's a bit messy. We could try to standardise this.

A bigger issue is with all the other packages. All of them base on gulp which isn't the most lightweight dependency and we use it to expose two simple tasks – lint and lint-staged.

So, every ckeditor5-* package depends on @ckeditor/ckeditor5-dev-lint, gulp, guppy-pre-commit which have around 380 dependencies: https://gist.github.com/Reinmar/d9633ea5bf583c6a85111a269d273995.

We install them using Lerna and only guppy-pre-commit is installed in every package physically. But it still takes a time to calculate all the deps for every package to see what needs to be hoisted to ckeditor5.

Another thing is that we have one additional file – gulpfile.js that we could remove if we switched to npm scripts. And, we could drop the ckeditor5-dev-lint package altogether if we'd be lucky.

So, the plan would be to try to implement:

• three scripts lint, lint-staged and pre-commit,
• automatic git pre-commit hook installation (just like guppy-pre-commit but triggering npm instead of gulp).

It seems that it can be easily done using https://github.com/okonet/lint-staged. We just need to figure out whether this will be any real improvement in terms of dependency bloat.

[Reinmar]

I checked that lint-staged, husky and eslint mean 200 deps (https://gist.github.com/Reinmar/56b38beec29c5ad6e5a60f2824642a39). It doesn't mean that they will really install faster when installed using Lerna, but there's a good chance for that.

I also thought about one more requirement – currently, our lint tasks ignore files listed in .gitignore but also one more directory (which is specified in gulpfile.js) – lib/**. We would also need to handle this somehow.

[Reinmar]

There's the sweet npx now which makes gulp even less needed.

[jodator]

So I've tested lint-staged with husky and example works (does eslint --fix on staged files) both in CLI and in PHPStorm (that was my biggest issue with guppy).

The only problem would be removing existing guppy git hook. Also from Husky README:

Existing hooks aren't replaced and you can use any Git hook.

So as I understand this would require to:

• rm -rf .git/hooks/pre-commit && rm -rf packages && npm update to remove pre-commit hook in ckeditor5 repo and the reinstall mgit dependencies.

[jodator]

After some trial and error we can add this to every project's package.json:

{
    "scripts": {
        "lint": "eslint",
        "precommit": "lint-staged"
    },
    "lint-staged": {
        "**/[^(lib)]**/*.js": [ "eslint --fix", "git add" ]
    }
}

with additional dependencies of lint-staged, husky and eslint thus removing guppy-pre-commit, gulp, @ckeditor/ckeditor5-dev-lint.

[Reinmar]

rm -rf .git/hooks/pre-commit && rm -rf packages && npm update to remove pre-commit hook in ckeditor5 repo and the reinstall mgit dependencies.

This will translate too (from the main project):

rm -rf .git/hooks/pre-commit
mgit exec 'rm -rf .git/hooks/pre-commit'
lerna clean && lerna bootstrap

[oleq]

I think that getting rid of guppy-pre-commit is a great idea because it breaks the workflow in applications like SourceTree and some IDEs too. There's no way to commit using these tools without changing manually the content of .git/hooks/pre-commit (AFAIR wrong paths, cwd, etc.).

I'd like to use SourceTree again to more than browsing and diffing—in such a distributed project, it's a must–have thing IMO.

[Reinmar]

Will you check if ST works fine with Husky (which replaces guppy) after @jodator will do the changes?

[jodator]

@Reinmar Husky works nice in PHPStorm so I guess that ST will work also.

[oleq]

Can you check it @jodator just to make sure? I'm in a totally different context at this moment.

[Reinmar]

BTW, please don't add --fix – its usage should be intentional. Otherwise, you might not notice that you changed some code.

[pomek]

And git add should not be called. We should manually add fixed files. From my point of view – I would like to see where I made a mistake (during git add -p I have such opportunity).

[Reinmar]

Actually, I realised that only part of the job was done. The other part is completely dropping gulp from the main package too.

So, let's continue the journey :)

We already have some scripts in the scripts/ directory so this will be nothing new.

[Reinmar]

It also seems that we can remove gulp from install-dependencies.sh in ckeditor5-dev-tests.

[jodator]

OK I've started checking scripts and it looks that gulp would be easy to remove thanks to a way how ckeditor5-dev scripts are build 👏. For now I can see that you'd only need to change how params are passed, ie:

gulp test --files=core
# is equal to:
npm test -- --files=core

ps.: I've found only one place that uses gulp and it's ckeditor5-dev-docs.

[Reinmar]

There will also be the dev env and testing env guides to update after such a change.

[Reinmar]

BTW, how will these calls look with npx? Is the -- separator needed too?

[pomek]

I'm not sure whether I understood your question but the -- separator is needed for distinguishing which parameters are for npm and which for executed command.

All parameters before -- modify the npm call. The rest parameters (after --) are passed to the executed command.

[jodator]

@pomek & @Reinmar - yep those -- before command params are needed.

[Reinmar]

I've been asking about npx (read more here: https://medium.com/@maybekatz/introducing-npx-an-npm-package-runner-55f7d4bd282b).

[pomek]

Npx… 😱

[jodator]

So the thing with npx is that it needs to find command in package. So some dev tools need to add "bin" to "package.json" so we can be able to use them.

So for tests we can have either:

npm t -- --files=core
# for installed script in .bin:
npx ckeditor5-dev-tests --files=core
# for command that npx cannot guess package:
npx ckeditor5-dev-tests-manual -p @ckeditor/ckeditor5-dev-test --files=core

setting node params:

# already defined in package.json
npm t
# must  be passed to npx
npx -n=--max_old_space_size=4096 ckeditor5-dev-tests
npx --node-arg=--max_old_space_size=4096 ckeditor5-dev-tests

Which for tests we can provide both: npm run-script and npx.

Other dev dependencies might be ported as well to support npx like translations:* gulp task would require to create binary ckeditor5-dev-transaltions and expose methods like collectTranslations as script param so we could run them as (currently methods are invoked in gulpfile.js:

npx ckeditor5-dev-translations -p @ckeditor/ckeditor5-dev-env collect

In the above if binary isn't named the same as package you must define from which package take binary.

Running npx commands works well if command has the same name as package:

npx @ckeditor/ckeditor5-dev-tests

Above will fetch @ckeditor/ckeditor5-dev-tests package and will run ckeditor5-dev-tests binary (and will fail due to memory requirements ;) )