Skip to content

When should dependencies be updated?#434

Merged
fgalan merged 9 commits intotelefonicaid:masterfrom
jason-fox:feature/update-deps
Sep 3, 2020
Merged

When should dependencies be updated?#434
fgalan merged 9 commits intotelefonicaid:masterfrom
jason-fox:feature/update-deps

Conversation

@jason-fox
Copy link
Copy Markdown
Contributor

@jason-fox jason-fox commented Jul 28, 2020

Not necessarily a PR, more of a question. Many dependencies on both prod and dev are getting increasingly updated. When and how do they get updated? Obviously changing dependencies has a much bigger impact than changing devDependencies, but devDependencies only is an indication of how far behind the package.json is:

   "devDependencies": {
-    "coveralls": "~3.0.2",
+    "coveralls": "~3.1.0",
-    "eslint": "~5.16.0",
+    "eslint": "~7.5.0",
-    "eslint-config-tamia": "~6.2.1",
+    "eslint-config-tamia": "~7.2.5",
-    "eslint-plugin-prettier": "~3.1.2",
+    "eslint-plugin-prettier": "~3.1.4",
-    "husky": "~1.1.0",
+    "husky": "~4.2.5",
-    "istanbul": "~0.4.5",
+    "nyc": "~15.1.0",
-    "lint-staged": "~7.3.0",
+    "lint-staged": "~10.2.11",
-    "mocha": "5.2.0",
+    "mocha": "8.0.1",
-    "moment": "~2.22.2",
+    "moment": "~2.27.0",
-    "nock": "10.0.2",
+    "nock": "13.0.3",
-    "prettier": "~1.14.2",
+    "prettier": "~2.0.5",
-    "remark-cli": "~6.0.1",
+    "remark-cli": "~8.0.1",
-    "remark-preset-lint-recommended": "~3.0.2",
+    "remark-preset-lint-recommended": "~4.0.1",
     "should": "13.2.3",
-    "sinon": "~9.0.1",
+    "sinon": "~9.0.2",
-    "textlint": "~11.0.1",
+    "textlint": "~11.7.6",
     "textlint-rule-common-misspellings": "~1.0.1",
-    "textlint-rule-no-dead-link": "~4.4.1",
+    "textlint-rule-no-dead-link": "~4.7.0",
-    "textlint-rule-terminology": "~1.1.30",
+    "textlint-rule-terminology": "~2.1.4",
     "textlint-rule-write-good": "~1.6.2",
-    "timekeeper": "2.1.2",
+    "timekeeper": "2.2.0",
     "watch": "~1.0.2"
   },

This PR for devDependencies only is split into three commits:

  1. Minimal change
  2. Documentation due to prettier
  3. Code Whitespace updates due to prettier.

Which of the changes above (if any) would make sense to raise as a series of PRs?
Given the recent es6 lint update, I think now may be an opportunity to update if you want the code whitespacing as well.

Package                             Current  Wanted   Latest
==========                            =====  ======   ======
coveralls                            3.0.14  3.0.14    3.1.0
eslint                               5.16.0  5.16.0    7.5.0
eslint-config-tamia                   6.2.1   6.2.1    7.2.5
husky                                 1.1.4   1.1.4    4.2.5
lint-staged                           7.3.0   7.3.0  10.2.11
mocha                                 5.2.0   5.2.0    8.0.1
moment                               2.22.2  2.22.2   2.27.0
nock                                 10.0.2  10.0.2   13.0.3
prettier                             1.14.3  1.14.3    2.0.5
remark-cli                            6.0.1   6.0.1    8.0.1
remark-preset-lint-recommended        3.0.4   3.0.4    4.0.1
textlint                             11.0.2  11.0.2   11.7.6
textlint-rule-no-dead-link            4.4.4   4.4.4    4.7.0
textlint-rule-terminology            1.1.30  1.1.30    2.1.4
timekeeper                            2.1.2   2.1.2    2.2.0
@fgalan
Copy link
Copy Markdown
Member

fgalan commented Jul 29, 2020

Sometimes dependencies are upgraded when github warns about some security issue but, apart from that, we don't have any rule on that. Thus, any PR upgrading dependencies without breakage of travis CI and without significative impact in code is welcome
:). Just a couple of (minor) comments:

In the particular case of this PR I have a doubt... is the upgrading of lint-related dependencies the cause of whitespace modifications? As far as I understand, we have configuration files for the linter which set our coding style, and they should prevail during dependencies upgrade processes like this one.

Comment thread package.json
Comment on lines +29 to +30
"test": "nyc --reporter=text mocha --recursive 'test/**/*.js' --reporter spec --timeout 3000 --ui bdd --exit",
"test:coverage": "nyc --reporter=lcov mocha -- --recursive 'test/**/*.js' --reporter spec --exit",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about this change... has the mocha and instanbul CLIs changes and new this new thing (nyc) is being used? Could you provide some more detail, please?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From https://github.com/istanbuljs/istanbuljs

  • nyc: the IstanbulJS 2.0 command line interface, providing painless coverage support for most popular testing frameworks.
  • babel-plugin-istanbul: a babel plugin
    for instrumenting your ES2015+ code with Istanbul compatible coverage tracking.
  • istanbul: the legacy 1.0 IstanbulJS interface (you should
    now consider instead using nyc or babel-plugin-istanbul).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NTC

@jason-fox
Copy link
Copy Markdown
Contributor Author

jason-fox commented Jul 29, 2020

I will add an entry to CNR

  • It would be desirable to upgrade all the IOTA related repos at once (IOTA Lib, IOTA Manager, IOTA-UL, IOTA-JSON, etc.).

Fine, except for IOTA Lib - it hasn't got prettier, eslint etc. yet. Not sure if you want telefonicaid/iotagent-node-lib#831 merged first.

When I raise a PR for IOTA Lib it will be a small one for Mocha, nyc etc, only - not prettier and eslint.
I'll raise a separate PR for upgrading elint/prettier and it can either be merged into 831 or actioned separately after 831 has reached master.

In the particular case of this PR I have a doubt... is the upgrading of lint-related dependencies the cause of whitespace modifications? As far as I understand, we have configuration files for the linter which set our coding style, and they should prevail during dependencies upgrade processes like this one.

This is to do with a bug fix in prettier there was a missing space between callback function definitions:

+ conn.on('error', function (err) {
- conn.on('error', function(err) {

which has changed. It cannot be configured.

@jason-fox
Copy link
Copy Markdown
Contributor Author

I'd be happy enough just updating the package.json version and getting that in first and raising a separate PR for the whitespacing if that is preferred. It is not a big issue and amending the whitespacing can be actioned incrementally if preferred. I guess it will depend on the state of the repos and the number of outstanding PRs in progress.

@fgalan
Copy link
Copy Markdown
Member

fgalan commented Jul 30, 2020

Fine, except for IOTA Lib - it hasn't got prettier, eslint etc. yet. Not sure if you want telefonicaid/iotagent-node-lib#831 merged first.

Right... I forgot it (my original comment has been amended)

This is to do with a bug fix in prettier there was a missing space between callback function definitions:

+ conn.on('error', function (err) {
- conn.on('error', function(err) {

which has changed. It cannot be configured.

Do you mean a bug in the prettier software itself, from 1.14.3 to 2.0.5? Why the preference of including a whitespace between the name of the function (when that function is used as callback) and the ( cannot be configured the same way we configure, for instance, the number of spaces to indent code lines or the preference to use spaces over tabs? Hard to understand the lack of flexibility in this specific aspect...

@jason-fox
Copy link
Copy Markdown
Contributor Author

Do you mean a bug in the prettier software itself, from 1.14.3 to 2.0.5?

Yes, this was a bug in 1.14.x, it has been fixed since and the missing whitespace added.

@jason-fox
Copy link
Copy Markdown
Contributor Author

I'd be happy enough just updating the package.json version and getting that in first and raising a separate PR for the whitespacing if that is preferred. It is not a big issue and amending the whitespacing can be actioned incrementally if preferred. I guess it will depend on the state of the repos and the number of outstanding PRs in progress.

I've reverted 8be8cae to reduce the size of the PR. If necessary a pass of prettier can be a separate PR, or it can be added incrementally as time goes by. Opinionated whitespacing in prettier isn't the main focus of this PR.

@jason-fox
Copy link
Copy Markdown
Contributor Author

jason-fox commented Jul 30, 2020

Looks like mqtt.org is still down. If this continues the link will need to be updated.

"Fixed" 5f518bd - mqtt.org is back.

Comment thread CHANGES_NEXT_RELEASE Outdated
jason-fox and others added 2 commits July 30, 2020 21:37
Co-authored-by: Fermín Galán Márquez <fgalan@users.noreply.github.com>
Copy link
Copy Markdown
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fgalan fgalan merged commit ff1ec4f into telefonicaid:master Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants