Remove `jake` dependency and use NPM scripts instead #5828

cherniavskii · 2017-10-07T21:05:42Z

removed jake dependency
removed test-jake NPM script (as duplicate of test script)
removed Jakefile.js
replaced Jake's build with NPM build script
updated docs

…omatically

mourner · 2017-10-11T15:35:20Z

CONTRIBUTING.md

+The Leaflet build system uses [Node](http://nodejs.org/).
+To set up the Leaflet build system, install [NodeJS](https://nodejs.org/en/).
+Note, that on Ubuntu/Debian you may need to [create a `node` symlink for nodejs package](https://stackoverflow.com/a/18130296).
+Then run the following commands in the project root to install dependencies:


Let's remove the Ubuntu/Debian mention and instead make sure you never need to call node directly, only through npm run.

I do not call node directly, but there is shebang line on the beginning of every script, that tells the system what interpreter to pass that file to (only works on Unix-like platforms, Windows just ignores that line and uses file extension to determine what executable should interpret it).

Note, that Jake also resolves node executable that way - see Jake's CLI.

As far as I understand, the line is only necessary if you call the script directly, e.g. ./script.js from the command line, which is not the way we suggest to run the scripts (since they're part of npm scripts). So removing the note should keep the guide clearer without consequences.

Actually, it's necessary even when calling script through NPM.

For example, you may want to run a bash script through NPM - and it shouldn't be executed by node. That's why #!/usr/bin/env node is necessary.

There is a mention about that in NPM docs (bottom of bin section).
More info about that shebang line.

The only Bash script we have in the Leaflet repo is publish.sh, and because it's Bash, it's irrelevant to the note about nodejs executable on Debian.

Bash script was just an example :)
I just wanted to clear that shebang line is necessary in order to tell NPM which executable should be used for particular script.
I added Ubuntu/Debian mention to docs only because of nodejs/node packages naming conflict on those platforms

Hi,

Trying to get this PR going through: if I understand correctly @mourner's comments:

remove the Ubuntu/Debian mention

…and…

keep the guide clearer

…the idea would be to have a simple guide, even if we have to let some specificities (like properly linking to node in this case) to the community to figure out (there is plenty resources on how to install node). IIRC, the standard install on Ubuntu installs node in the path, so this manual step would not be the common case. Hence we could reasonably keep it out indeed.

As for the shebang line added by @cherniavskii, it is necessary in order to have the file executed by node without having to call node explicitly in the CLI. However, as noted by both @mourner and @cherniavskii, since all these scripts are actually called as npm package scripts, we can simply include the node call in those scripts, and avoid having this shebang line cause other troubles.

E.g.:
package.json

"docs": "node ./build/docs.js"

What do you think?

@ghybs I didn't know about those troubles using shebang line.
Indeed, calling node in npm scripts sounds reasonable.

the standard install on Ubuntu installs node in the path

Yes, I confirm. I have installed node on Ubuntu lately and node alias works just fine.

I'll remove shebang lines and update docs.
Thanks!

@ghybs done!

@cherniavskii thank you for the refactoring! 👍

Just to be fair regarding the shebang line again, even though the above link mentions some possible issues, most npm module binaries include such a shebang line, as you noted. Therefore it is probably a very corner case…

mourner · 2017-10-11T15:37:24Z

scripts/test.js

+		CLIEngine = require('eslint').CLIEngine,
+		eslintCli = new CLIEngine(),
+		karma = require('karma'),
+		testConfig = {configFile : path.join(__dirname, '../spec/karma.conf.js')};


While we're at it, could we simply get rid of this script fully? Add separate NPM scripts for ESLint and use the default Karma CLI.

Then we'd only be left with scripts for building things, so we'd leave the files in the build directory.

Good point.
This solution will have a tiny drawback - lack of custom console logs (like Checking for JS errors... before linting source code).
Personally, I'm okay with that, since logs still would be readable enough to find out which script succeeded / failed.
But there is always an option to log this info using bash commands.
What do you think about that?

I think it's enough to have various steps in separate npm scripts — that way the relevant command will be printed before execusion so you know what's going on (e.g. > npm run lint-spec).

mourner · 2017-10-17T14:32:12Z

.travis.yml

+script:
+  - npm run lint
+  - npm run lint-spec
+  - npm test


I would put lint and lint-spec to pretest script so that they're called automatically before npm test. And it will be consistent with the current behavior where npm test does all the necessary checks.

When tests in real browsers will be added to Travis, I would like to have separate test script for each browser (for example npm test --browsers Firefox and npm test --browsers Chrome instead of npm test --browsers Firefox,Chrome) to avoid mixing logs from different browsers (for better readability).
Adding lint and lint-spec to pretest script will trigger linting for each test run, which makes no sense.

From the other hand, having linting scripts inpretest would be handy for local development.

What solution do you prefer?

Hi @cherniavskii,

While having lint check on its own script to avoid duplication in Travis is nice, it is true that executing it automatically with every npm test command during dev is much more comfortable.
Having the lint triggered for each browser in Travis does not harm much.

If you really want to avoid the duplication, we could imagine configuring Travis to use another command that does not include the lint phase:

package.json:

"pretest": "npm run lint && npm run lint-spec", "test": "npm run test-nolint", "test-nolint": "karma start ./spec/karma.conf.js"

.travis.yml:

script: - npm test - npm test-nolint --browsers Firefox - etc.

What do you think?

@ghybs thanks for your comment!

Having the lint triggered for each browser in Travis does not harm much.

It's true, but if it's possible to avoid this duplication - I'd like to do it :)
I really like solution you proposed! Will update PR!

Klortho · 2018-01-01T17:03:58Z

What's wrong with Jake?

mourner · 2018-01-05T10:01:12Z

@Klortho for starters, it's simply unnecessary. You can see how much code the PR removes. Also, I find simple NPM scripts easier to maintain, and they are more familiar to new contributors.

Run linting scripts in pretest script, this way test script will automatically trigger code linting. Add test-nolint script to have possibility to run tests without triggering pretest script with linting. This is useful when running tests in different browsers and tere's no sense to lint code more than one time

ghybs · 2018-01-25T15:05:13Z

@mourner I think your previous change requests have been addressed now…
Please do you have other concerns before we can proceed?

PR modified according to the requested changes

ghybs

Nice simplification of development dependencies!

IvanSanchez

Me like. 👍

cherniavskii added 11 commits September 30, 2017 15:56

run docs via npm

b0e44b9

add shebang line to integrity.js, so npm will resolve node binary aut…

5861c0d

…omatically

remove unnecessary build-jake npm script

4db4fc9

add test.js script as replacement of Jakefile.js

deb2d28

remove Jakefile, remove jake from devDependencies

3a7dd07

update docs

e384f79

move scripts to separate directory

b5f65d3

change shebang line from nodejs to js

597418c

change shebang line from js to node

9c0f9d4

Merge branch 'master' into migrate-to-npm-scripts

7162a2e

update docs

1bf2d14

mourner requested changes Oct 11, 2017

View reviewed changes

use Karma cli instead of custom test script

5ac694a

mourner previously requested changes Oct 17, 2017

View reviewed changes

cherniavskii added 4 commits January 24, 2018 13:34

Merge branch 'master' into migrate-to-npm-scripts

d075dfa

replace shebang lines with node executable in npm scripts

4254bcc

remove note about nodejs symlink in CONTRIBUTING.md

d036e37

ghybs approved these changes Feb 1, 2018

View reviewed changes

IvanSanchez approved these changes Feb 1, 2018

View reviewed changes

ghybs merged commit e3b049c into master Feb 5, 2018

cherniavskii deleted the migrate-to-npm-scripts branch February 5, 2018 10:54

ghybs mentioned this pull request Oct 9, 2018

Docs(contrib): fix command to run test with karma options #6363

Merged

snyk-bot mentioned this pull request Sep 22, 2019

[Snyk] Upgrade leaflet from 1.3.1 to 1.5.1 Matthelonianxl/lisk-explorer#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `jake` dependency and use NPM scripts instead #5828

Remove `jake` dependency and use NPM scripts instead #5828

cherniavskii commented Oct 7, 2017 •

edited

Loading

mourner Oct 11, 2017

cherniavskii Oct 14, 2017 •

edited

Loading

mourner Oct 16, 2017

cherniavskii Oct 16, 2017

mourner Oct 17, 2017

cherniavskii Oct 17, 2017

ghybs Jan 25, 2018

cherniavskii Jan 25, 2018

cherniavskii Jan 25, 2018

ghybs Jan 25, 2018

mourner Oct 11, 2017

cherniavskii Oct 14, 2017

mourner Oct 16, 2017

cherniavskii Oct 16, 2017

mourner Oct 17, 2017

cherniavskii Oct 17, 2017 •

edited

Loading

ghybs Jan 22, 2018

cherniavskii Jan 24, 2018 •

edited

Loading

Klortho commented Jan 1, 2018

mourner commented Jan 5, 2018

ghybs commented Jan 25, 2018

ghybs left a comment

IvanSanchez left a comment

Remove jake dependency and use NPM scripts instead #5828

Remove jake dependency and use NPM scripts instead #5828

Conversation

cherniavskii commented Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

cherniavskii Oct 14, 2017 • 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

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

cherniavskii Oct 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cherniavskii Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Klortho commented Jan 1, 2018

mourner commented Jan 5, 2018

ghybs commented Jan 25, 2018

ghybs left a comment

Choose a reason for hiding this comment

IvanSanchez left a comment

Choose a reason for hiding this comment

Remove `jake` dependency and use NPM scripts instead #5828

Remove `jake` dependency and use NPM scripts instead #5828

cherniavskii commented Oct 7, 2017 •

edited

Loading

cherniavskii Oct 14, 2017 •

edited

Loading

cherniavskii Oct 17, 2017 •

edited

Loading

cherniavskii Jan 24, 2018 •

edited

Loading