Add stylelint and prettier

[KristjanESPERANTO]

Here is my suggestion to introduce stylelint and prettier. I prefer the tools' defaults as much as possible, so there are a lot of style changes in this PR. If you have other preferences, I'm happy to change the rules accordingly.

There is now a new command to autofix lint issues: npm run lint:fix.

Functionally there are no changes.

[BePo65]

You are right: when creating pr #113 I missed the styles (.leaflet-container:-webkit-full-screen and .leaflet-container:-ms-fullscreen).

Regarding .leaflet-container:full-screen: I didn't find an reference for that style, so I suppose it is not required (mysteriously even in version 2.x and 1.x there is no reference to this class; so perhaps it was never required).

I love this pr very much - even better than my own pr #116 (which I created before I saw yours). But I have 2 points that could be changed to make it even better:

1. you changed .eslintrc to .eslintrc.json to conform to the eslint recommendation, but you dropped the 'rules' and the 'globals' settings - why?
2. I recommend using npx, when calling eslint in the package.json script - this make this script usable for *nix users and for windows users alike.

[KristjanESPERANTO]

Short answer

I'm glad you like my PR. I made a few adjustments based on your comments.

Long answer

1.1. Rules - Firstly I think it only makes sense to list rules that have an effect. And secondly I prefer the standard settings of prettier and eslint - and only want to define exceptions to them if necessary. I'll go into each rule:

• "camelcase": 0 - This rule has no effect on our current code regardless of whether it is enabled or not. If we enable it, it would enforce camelCase on new code, ensuring consistency. I think that's a good rule. Since the rule is enabled by default, we don't need to list it here.
• "quotes": [2, "single", "avoid-escape"] - This rule is a matter of taste. I don't see any advantage in deviating from the standard settings.
• "no-mixed-spaces-and-tabs": [2, "smart-tabs"] - This rule is unnecessary as prettier would already report this as an error.
• "space-before-function-paren": 2 - Activating this rule here is unnecessary as it is activated by default.
• "space-in-parens": 2 - This rule is a matter of taste. I don't see any advantage in deviating from the standard settings.
• "object-curly-spacing": [2, "never"] - This rule is a matter of taste. I don't see any advantage in deviating from the standard settings.
• "array-bracket-spacing": 2 - This rule is a matter of taste. I don't see any advantage in deviating from the standard settings.
• "computed-property-spacing": 2 - This rule is a matter of taste. I don't see any advantage in deviating from the standard settings.
• "space-before-blocks": 2 - Activating this rule here is unnecessary as it is activated by default.
• "keyword-spacing": 2 - This rule is unnecessary as prettier would already report this as an error.
• "no-lonely-if": 2 - Okay, I missed that rule. This is not activated by default and is not covered by prettier. So I included it in my PR.
• "comma-style": 2 - This rule is unnecessary as prettier would already report this as an error.
• "no-underscore-dangle": 0 - Disabling this rule is not necessary here as it is disabled by default.
• "no-constant-condition": 0 - Disabling this rule is not necessary here as it is disabled by default.
• "no-multi-spaces": 0 - This rule has no effect on our current code regardless of whether it is enabled or not. Prettier would report it as an error (And I think that's a good thing), so this rule is unnecessary.
• "strict": 0 - This rule has no effect on our current code regardless of whether it is enabled or not. I don't think it makes sense to deactivate it.
• "key-spacing": 0 - This rule has no effect on our current code regardless of whether it is enabled or not. Prettier would report it as an error (And I think that's a good thing), so this rule is unnecessary.
• "no-shadow": 0 - This rule has no effect on our current code regardless of whether it is enabled or not. I don't think it makes sense to deactivate it.
• "no-unused-vars": 2 - Activating this rule here is unnecessary as it is activated by default.
• "curly": 2 - Activating this rule here is unnecessary as it is activated by default.
• "no-unused-expressions": [2, {"allowShortCircuit": true}] - Okay, you can activate the rule. In our code, however, it makes sense to replace allowShortCircuit with allowShortCircuit.
• "spaced-comment": 2 - Activating this rule here is unnecessary as it is activated by default.

1.2. globals - As I understand it, defining globals only makes sense if the no-undef rule is also activated. It wasn't, so I removed the globals. But I didn't really think that through, it's better to activate the no-undef rule. I've now done that and added the globals again accordingly.

2. npx - Okay, I only use Linux and didn't know that this is an issue with windows. I added npx to the scripts :slightly_smiling_face:

[KristjanESPERANTO]

I examined .leaflet-container:-webkit-full-screen again. If we remove that, we would have problems with Safari because Safari doesn't yet support the css selector fullscreen: https://caniuse.com/mdn-css_selectors_fullscreen

So I reinserted the part and added a comment with the last commit: https://github.com/brunob/leaflet.fullscreen/pull/115/commits/57dd1011ea513f10403231c3ea7fc7a66077a949.

[BePo65]

Concerning the eslint rules:

I tried to change as little as possible on the original rule definitions and just added those rules from the leaflet repo that add new "error" rules. IMHO the eslint configuration used by the leaflet repo has some topics of its own, but this is a leaflet plugin, so I wanted to be as compatible with their definition as possible.

Long answer short: I also prefer using predefined rule sets and if you prefer prettier, then yours is IMHO a good selection of rules - stay with that.

And thank you for such a positive reaction on my ideas (and sorry again for the 'npx' thing).

[brunob]

Thx for this work ! I will not go in the debate around tabs VS spaces ;o but why this PR changes ' to " in all js parts ?

[BePo65]

The quotes are a result of the way prettier defines its rules. They say that their style results in less escapes (see prettier documentation).

[KristjanESPERANTO]

I have a clear position here (preferring the prettier and eslint defaults), but I have no hard feelings if you both want it differently 🙂 For example, we can also convince prettier to accept single quotes.

[brunob]

I have a clear position here (preferring the prettier and eslint defaults), but I have no hard feelings if you both want it differently 🙂 For example, we can also convince prettier to accept single quotes.

I would prefer single quotes, as it's done in Leaflet core :)

[KristjanESPERANTO]

I would prefer single quotes, as it's done in Leaflet core :)

Alright, done 🙂

[KristjanESPERANTO]

Is something still missing here or should I change something?

[brunob]

Thx for updating this one, i was sorry to push changes on master for that ! I think this could be merge, let me read that again next days, merge it and publish a release to promote your work :)

[brunob]

Sorry for the delay, i have some doubts :

• every Leaflet project use tabs for indentation
• Leaflet and its plugins seems to use "only" eslint with "mourner" config see https://github.com/Leaflet/Leaflet.heat/blob/gh-pages/package.json#L26

Concerning eslint config, i have to admit that every plugin use its own config, ftr on https://github.com/leaflet-extras/leaflet-providers/blob/master/.eslintrc we use another different one :\

Do you think that it would be better to stick to Leaflet "rules" or on the ones proposed here ?

[BePo65]

'eslint-config-mourner' sets a lot of rules. Concerning the stylistic rules I would follow the recommendations of the eslint team.

'prettier' has the advantage that it has nearly no configurations options and thus defines a fixed set of rules.

An IMHO the same goes for the eslint rules. Anyone has good reasons to set a rule one way or the other. But for libraries like this, I think staying with the default ('recommended') rule set makes discussions easier.

[KristjanESPERANTO]

Concerning eslint config, i have to admit that every plugin use its own config, ftr on https://github.com/leaflet-extras/leaflet-providers/blob/master/.eslintrc we use another different one :\

Yeah, there are many config variants for ESLint, but: The previous variants will be deprecated with version 9 of ESLint. Then the flat config (in a file named eslint.config.js) is the way to go. When I created the PR, I didn't realize that yet. In the meantime, I am migrating my projects towards flat config. The flat config already works in version 8 of ESLint.

Do you think that it would be better to stick to Leaflet "rules" or on the ones proposed here ?

Like @BePo65, I think the best way is to use ESLint's recommendations as a basis. Of course, we can still define some exceptions.

A lot is happening at ESLint at the moment. Besides the flat config, they are in the process of separating the rules from the core and putting them into two separate packages (one for "real linter rules" and one for stylistic rules).

Even if it is a pity about the previous work on this PR, my suggestion would be that I (or someone else) create a new PR that uses the new flat config and activates the recommended rules. If we don't like some of the effects, we define exceptions. What do you think?

[brunob]

Great, thx for your detailed comment, i trust you both on the implementation to choose, gogogo :)

One last think, i think we should keep tabs for indentation as other leaflet projects.

[KristjanESPERANTO]

Okay, there's a new PR: #119 :rocket: Therefore I'm closing this one.

I may not have covered all the requirements mentioned here. Please give me feedback in the new PR :slightly_smiling_face: