Should every IoT Agent align to use eslint

[jason-fox]

LoRaWAN and OPC-UA already use ES6 and are linted using ESLint. The telefonica IoT Agents code base is older and continues to use jshint

I see several advantages of swapping out the older linter and replacing it with the ES6 compliant one:

1. Consistency - users would find it easier to navigate between the various projects.
2. An Es6 codebase is more modern and likely to attract more contributions.
3. EsLint can be configured to check code only and delegate formatting rules to work with prettier to avoid the need for extra jshint workarounds in the code base.
4. Use of constructs such as const and let is supposedly more performant - well they would say that wouldn't they

The existing minimum node engine is Node 8. Node 8 is reaching EoF and is already compliant with const and let anyway . I can't see why there would there be any problem is upgrading the IoT Agents to use the newer syntax? It's just a start - ideally someone can delete the existing async library callbacks and replace them with proper native ES2015+ Promises and use async await - it would make the code a lot easier to follow. I'm not sure if the iot-node-lib eventually needs to offer promise endpoints itself or just use utils.promisify() but that is an issue for another day. This would just be a start.

As a broad mechanical automated change, it would make sense to try push this through whilst there is a lull to avoid merge conflicts with other ongoing PRs.

[jason-fox]

I'd like to swap out JSHint for EsLint on the long runner: #753 as well [sigh] since it should make the maintenance of the ongoing merge conflicts easier. If this proposal is accepted I may combine prettier and eslint into a single PR (and remove all jshint work arounds.

[jason-fox]

Related PRs:

• Feature/eslint iotagent-json#456 ✅
• Re-run eslint iotagent-ul#415 ✅
• Feature/eslint sigfox-iotagent#70 ✅
• Feature/eslint lightweightm2m-iotagent#215 ✅
• Update to es6, add lint staged and husky iotagent-manager#196 ✅
• Update to es6 fiware-sth-comet#529 ✅

Still Open:

• Feature/eslint #831

[fgalan]

Thank you very much for creating the issue and the explanations above. We are still discussing the issue internally and I hope we can provide feedback on it soon. Sorry for the delay.

On the meanwhile, looking to the proposed PRs, I have a couple of doubts, pls:

• What is the relation between tamia and eslint? (as far as I understand eslint is a linter that check the code against a set of rules, but I don’t know what is tamia)
• Could you provide the reference document describing the options in .eslintignore?

[jason-fox]

To quote Wikipedia:

eslint is a static code analysis tool for identifying problematic patterns found in JavaScript code. Rules in ESLint are configurable, and customized rules can be defined and loaded

Tâmia provides a set of such rules

• Tab indentation.
• Single-quotes.
• Semicolons.
• Declare variables just before their first usage.
• Multiple variable statements.
• Make const, not var.
• Use === and !== over == and !=.
• Return early.
• Limit line lengths to 80 chars.
• Prefer readability over religion.
• Use ES6.

This is very similar to the commonly used airbnb code rules, with one important addition/omission/change. - Tâmia does not provide any code style rules, hence it is very easy to complement this base set of linter rules with another for code styling.

As you know, I've been pushing Prettier - and they also supply a eslint config eslint-plugin-prettier - using this enforces the prettier formatting, and removes the need for any overrides where jshint formatting rules were conflicting with prettier.

The .eslintignore and .eslintrc files are described in the config documentation

[fgalan]

Please find some feedback:

• We understand the reasons behind linting and we agree that it can improve the code. However, for us this is "nice to have". I mean, it has less priority than another kind of contributions (new features, bugfixing, etc.). The impact of the new PRs doing an overall syntax change (thousands of lines of code changed in the same PR) on existing contributions should be carefully analyzed. They should be merged in lull moments, as you said. For IOTAs repositories, maybe we have that kind of lull, but not so sure in the case of iotagent-node-lib, where some key contributions are yet ongoing (for instance: Provide an alternative expression plugin #800). Explicit agreement (i.e. LGTM mark on the syntax change PR) from the authors of potentially conflicting PRs would be great.
• With regards to consistency, take into account that IOTA Manager (https://github.com/telefonicaid/iotagent-manager) is also part of the IOTA suite (although it seems it is not part of FIWARE as GE). Thus, if we at the end implement ES6 related changes and move linter to eslint, an analogous PR on that repository should be also created.
• With regards to the change from async library callbacks to promises and async await I’d suggest not to include that changes in the existing PRs. Different to the changes already included (var to let/conts, etc.), which seems to be pretty straightforward (to be confirmed upon pull request review), the changes from callbacks to promises is more profound. We need to evaluate the pros/cons. It would be great to create a small PR (just one file, the one which best could show the supposed advantages with regards a code cleanness) to do that evaluation. That small PR could be done now (in parallel to the existing big ones) or in a next round of PRs in a later time.

[jason-fox]

I have created my own branch of #800 - expression-lib-alternative and merged my eslint branch into it:

1. There is no conflict when running the initial merge - ff62d37
2. Running npm run lint prettifies the code as shown: b59d0c2
3. Fixing the remaining errors is merely replacing jshint overrides with eslint ones - c3899f7

Total time to fix - 8 minutes - I don't think this is much of a blocker.

Replacing jshint with eslint is a simple mechanical process which could be easily added into IOTA Manager - the only problem is waiting and merging in other PRs whilst still at the back of the queue takes time. If I could see movement elsewhere I could easily apply the same process to another repo.

I fully agree removing callbacks and using promises is an aspiration and not intended to be part of this PR and should be separated from this discussion.

[jason-fox]

This is the state of the lull across the IoT Agents

Name	Open PRs	Comments
iotagent-json	3	One from April, A documentation one and a new change
iotagent-ul	3	Two PRs are 11 months old - are they active? One from this month.
sigfox-iotagent	1	A documentation change from March - is it active?
lightweightm2m-iotagent	1	One 18 months old - is it active?

Name	Open PRs	Comments
iotagent-node-lib	10	Includes several documentation changes, a Docker change and the obsolete prettier PR
iotagent-manager	1	One outstanding PR

Which of these PRs are still active?

[fgalan]

Tab indentation.

As far as I remember we use plain spaces to indent in our code, not tabs. Can configuration be adjusted to align with our current indentation style?

In fact, maybe already works that way... I see in the proposed PRs configuration like:

indent_style = tab
...
indent_size = 4 

What does exactly mean?

(I have tried to find indent_style and indent_size in the link you provide https://eslint.org/docs/user-guide/configuring but I didn't find them...)

[jason-fox]

My mistake - that should be the editorconfig - an attempt to maintain consistent coding styles across various IDEs.

The JavaScript editorconfig should indeed match the 4 space guideline (and align with prettierrc.json)

[*.{js}]
indent_style = space
indent_size = 4

[jason-fox]

Given that #800 has now been merged, I thought it would be an opportune moment to ask whether these PRs could be processed. I have updated the PRs again to ensure no conflicts.

This is the state of the lull across the IoT Agents

Name	Done	Open PRs	Comments
iotagent-json	✅	3	A documentation one and "ignore measure" - updated and currently ready for review.
sigfox-iotagent	✅	1	A non-conflicting documentation change from March - is it active?
lightweightm2m-iotagent	✅	1	One 2 years-old - is it active?
iotagent-manager	✅	1	One outstanding PR - 8 months old

For these repos I see no conflicting commits in the last 6 months and no progress on the outstanding PRs - are they active? How is the queue being dealt with?

Name	Done	Open PRs	Comments
iotagent-node-lib	❌	10	Includes several documentation changes, a Docker change and the obsolete prettier PR
iotagent-ul	✅	8	Two PRs are 11 months old - are they active? One from this month.

These two are more problematic, and I can see you will want to process #849 before accepting #831. Question should #753 be withdrawn? The eslint PR supersedes it.

On the UL IoT Agent all the outstanding PRs telefonicaid/iotagent-ul#411 and telefonicaid/iotagent-ul#412 appear to be waiting for some equivalent works - is this scheduled? The other two are approved or equivalents to approved work elsewhere - telefonicaid/iotagent-ul#429 telefonicaid/iotagent-ul#430

Basically just asking if it is worth continuing maintaining these PRs.

[fgalan]

I'm still thinking the work is valuable. Let's start the merge by the "simplest" repositories which I think are Sigfox and L2M2M.

The PR in LWM2M has been just merged. The PR in Sigfox has been commented with a couple of things I have spotted in a last review on it.

[fgalan]

Current status: eslint PRs on IOTA-Sigfox, IOTA-LWM2M and IOTAManager (and STH, as bonus track ;) has been already merged. Next ones are IOTA-UL and IOTA-JSON.

There is an outstanding ongoing work on leading slashes mqtt topics authored by @jmezzera. The piece of work corresponding to IOTA-JSON is pending, but given that the "parallel" work in IOTA-UL is finished (PR telefonicaid/iotagent-ul#429) I understand it would be provided soon. @jmezzera could you provide some update on this, please? Thanks!

[jmezzera]

I was just working on this aspect this morning, planning to have it done by the end of the day.

[jmezzera]

Done telefonicaid/iotagent-json#481

[fgalan]

@jmezzera work on MQTT improvements has been just merged in IOTA-UL and IOTA-JSON (thanks!). Thus, I think once the eslint PRs on these repos gets updates with master and conflict solved, they would be ready for final review and merge.

[jason-fox]

Conflicts have been resolved. PRs have been updated.

• Ultralight - Re-run eslint iotagent-ul#415 ✅
• JSON - Feature/eslint iotagent-json#456 ✅

[fgalan]

As far as I understand, all the PRs related with eslint have been already merged in every repository so we are almost done :) However, there is still one pending, in this repository itself, for the iotagent-node-lib: PR #831.

The summary of "parallel" PRs that may conflict are the following ones:

PR number	Author	Does the author agree on merging PR #831 before his PR?
#601	@m4n3dw0lf	? (author asked)
#674	@gabrieledeluca	Closed (overpassed by PR #920)
#688	@fgalan	Yes
#753	@jason-fox	Withdrawn
#780	@Jagatjot	Closed (ported to #922)
#920	@jason-fox	Merged
#922	@fgalan	Yes
#793	@Cerfoglg	Yes
#849 (and dependant #854)	(*)	Yes

Notes:

• It would be great if we could get feedback from every author to "solve" the "?" in either "Yes" or "No". Please, for any of the authors, edit the table itself to include your answer (or, if not possible due to permissions in github, provide feedback as comment in this issue and I'll update the table based on that).
• When you provide that feedback take into account the procedure that you would need to apply if the PR Feature/eslint #831 is merged before yours. It is described here and, from my point of view, is pretty easy and straighforward.
• With regards to [e2e testing] NGSI-LD #849, github shows me as author of the PR. However, I just did an aggregation work from PRs contributed by @jason-fox , so I prefer he provides feedback on that row.
• With regards to PR Add prettier code formatting #753, @jason-fox says should Add prettier code formatting #753 be withdrawn? The eslint PR supersedes it. I don't have the differences in detail, but if PR Feature/eslint #831 includes the work done in [MUST] Show versioned documentation #754 (or, ini other words, [MUST] Show versioned documentation #754 is a subset of Feature/eslint #831) then withdrawal seems to be a good idea.

[jason-fox]

• Add prettier code formatting #753 (prettier) has been withdrawn - Feature/eslint #831 does everything and more (prettier + husky + eslint)

• [e2e testing] NGSI-LD #849 (198 files!) should definitely be merged before processing Feature/eslint #831 . The justification is simple. The NGSI-LD work involves a very large number of files and includes an extensive refactoring - it currently has some minor conflicts with master - usually just whitespace or documentation. It would be simple to merge at the moment. [e2e testing] NGSI-LD #849 already uses es6 for the code in relevant directories. The however the branch is currently several PRs behind master.

• For NGSI-v2, ensure GeoJSON is correctly encode in the request. #854 (24 files) is of moderate size should probably be merged before processing Feature/eslint #831 easiest would be to either:

  • merge geojson => ngsi-ld and then ngsi-ld => master
  • merge ngsi-ld => master and then geojson => master

#831 is a blind mechanical change - it will ES6-ify the existing master code which adds unnecessary changes over the currently ngsi-ld refactored (and already ES6-ified) files. This means a subsequent master => ngsi-ld merge to resolve conflicts would include a lot more unnecessary stuff - it will be very tricky to remove conflicts since each file will need analysis especially as I haven't looked at the code in either of these PRS in over six months.

Much easier to merge the ngsi-ld => master stuff first and then merge eslint => master after. Eslint is already up to date with master so it will be a matter of accept theirs, much easier not to miss something.

In contrast none of the other PRs are large enough to cause too many problems ( 9, 3, 1 and 10 files respectively)

[fgalan]

Thanks for the feedback! I have edited the table above accordingly.

[jason-fox]

I have fixed the merge conflicts for the NGSI-LD PR:

• Conflict resolution master => Feature/842 ngsi ld #881

This makes the merge of ngsi-ld + eslint a simple matter of accept mine as an experiment have created a new branch for this see here: https://github.com/jason-fox/iotagent-node-lib/tree/feature/eslint_and_ngsi-ld

I would still prefer that the NGSI-LD is actioned first as I don't want to be maintaining three branches (eslint, ngsi-ld , eslint+ngsi-ld) whilst waiting for merge to master

[fgalan]

PR #831 finally merged.

After merging, let's see tonight if the newly generated IOTAs containers with the new version of the library work in the CI e2s tests. If no problem is found (we don't expect any), the issue will be closed.

[fgalan]

After merging, let's see tonight if the newly generated IOTAs containers with the new version of the library work in the CI e2s tests. If no problem is found (we don't expect any), the issue will be closed.

CI e2e went fine last night. So I think nothing is pending and this issue can be closed.