search

readium / r2-testapp-js

NodeJS Readium2 "test app"
BSD 3-Clause "New" or "Revised" License
10 stars 1 forks source link

NPM ci / install, major issues with github, git+ssh/http[s] dependencies #10

Closed danielweck closed 5 years ago

danielweck commented 5 years ago

MacOS, Windows, Linux ... NPM v5 / v6, I keep having major problems building and running r2-testapp-js and readium-desktop depending on how the node_modules folder hierarchy is created / maintained by NPM.

This is an old problem: https://github.com/npm/npm/issues/5123

NPM dedupe does not suffice, but I found that this utility script does a good job (I patched it to support github in addition to git[+ssh|http[s]]): https://github.com/kazuho/force-dedupe-git-modules ( https://www.npmjs.com/package/force-dedupe-git-modules )

The good new is that we have control over ta-json (and therefore its dependency reflect-metadata), css-element-queries, and riot-typed ... however ttfinfo is required by system-font-families, so we have to see if this is problematic.

Bottom line: we really have to eliminate the Git(Hub) dependencies in our NPM package.json files, across the board (i.e. replace them with proper npmjs.com published deps).

danielweck commented 5 years ago

On Linux Lubuntu with NodeJS 8.12 and NPM 6.4.1, I am observing that npm ci works (no need to fix node_modules using the force-dedupe-git-modules utility script), whereas npm install creates a bogus node_modules hierarchy with duplicate packages. From prior experience, I know that NPM on Windows and MacOS behaves differently (notably, I've not had any problems at all on MacOS).

danielweck commented 5 years ago

A temporary solution would be to introduce the force-dedupe-git-modules utility (well, my patched version) as a postinstall script in package.json, but I would prefer to replace Git(Hub) dependencies with proper NPM semver packages.

danielweck commented 5 years ago

Here is what npm install does to package-lock.json (obvious NPM package moves from top-level node_modules to inner dependencies ... thus the duplicates):

diff --git a/package-lock.json b/package-lock.json
index 0a802ba..1e0daad 100644
--- a/package-lock.json
+++ b/package-lock.json
@@ -2270,10 +2270,6 @@
         "coffee-script": "^1.10.0"
       }
     },
-    "css-element-queries": {
-      "version": "github:marcj/css-element-queries#e1f3e0475a52b830d4b6490abd7b2984fbc8553f",
-      "from": "github:marcj/css-element-queries#master"
-    },
     "css2json": {
       "version": "0.0.4",
       "resolved": "https://registry.npmjs.org/css2json/-/css2json-0.0.4.tgz",
@@ -3643,11 +3639,13 @@
         },
         "balanced-match": {
           "version": "1.0.0",
-          "bundled": true
+          "bundled": true,
+          "optional": true
         },
         "brace-expansion": {
           "version": "1.1.11",
           "bundled": true,
+          "optional": true,
           "requires": {
             "balanced-match": "^1.0.0",
             "concat-map": "0.0.1"
@@ -3660,15 +3658,18 @@
         },
         "code-point-at": {
           "version": "1.1.0",
-          "bundled": true
+          "bundled": true,
+          "optional": true
         },
         "concat-map": {
           "version": "0.0.1",
-          "bundled": true
+          "bundled": true,
+          "optional": true
         },
         "console-control-strings": {
           "version": "1.1.0",
-          "bundled": true
+          "bundled": true,
+          "optional": true
         },
         "core-util-is": {
           "version": "1.0.2",
@@ -3771,7 +3772,8 @@
         },
         "inherits": {
           "version": "2.0.3",
-          "bundled": true
+          "bundled": true,
+          "optional": true
         },
         "ini": {
           "version": "1.3.5",
@@ -3781,6 +3783,7 @@
         "is-fullwidth-code-point": {
           "version": "1.0.0",
           "bundled": true,
+          "optional": true,
           "requires": {
             "number-is-nan": "^1.0.0"
           }
@@ -3793,17 +3796,20 @@
         "minimatch": {
           "version": "3.0.4",
           "bundled": true,
+          "optional": true,
           "requires": {
             "brace-expansion": "^1.1.7"
           }
         },
         "minimist": {
           "version": "0.0.8",
-          "bundled": true
+          "bundled": true,
+          "optional": true
         },
         "minipass": {
           "version": "2.2.4",
           "bundled": true,
+          "optional": true,
           "requires": {
             "safe-buffer": "^5.1.1",
             "yallist": "^3.0.0"
@@ -3820,6 +3826,7 @@
         "mkdirp": {
           "version": "0.5.1",
           "bundled": true,
+          "optional": true,
           "requires": {
             "minimist": "0.0.8"
           }
@@ -3892,7 +3899,8 @@
         },
         "number-is-nan": {
           "version": "1.0.1",
-          "bundled": true
+          "bundled": true,
+          "optional": true
         },
         "object-assign": {
           "version": "4.1.1",
@@ -3902,6 +3910,7 @@
         "once": {
           "version": "1.4.0",
           "bundled": true,
+          "optional": true,
           "requires": {
             "wrappy": "1"
           }
@@ -4007,6 +4016,7 @@
         "string-width": {
           "version": "1.0.2",
           "bundled": true,
+          "optional": true,
           "requires": {
             "code-point-at": "^1.0.0",
             "is-fullwidth-code-point": "^1.0.0",
@@ -6876,9 +6886,22 @@
         "r2-utils-js": "^1.0.0-alpha.4",
         "request": "^2.88.0",
         "request-promise-native": "^1.0.5",
-        "ta-json": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
         "tslib": "^1.9.3",
         "urijs": "^1.19.1"
+      },
+      "dependencies": {
+        "reflect-metadata": {
+          "version": "0.1.12",
+          "resolved": "https://registry.npmjs.org/reflect-metadata/-/reflect-metadata-0.1.12.tgz",
+          "integrity": "sha512-n+IyV+nGz3+0q3/Yf1ra12KpCyi001bi4XFxSjbiWWjfqb52iTTtpGXmCCAOWWIAn9KEuFZKGqBERHmrtScZ3A=="
+        },
+        "ta-json": {
+          "version": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
+          "from": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
+          "requires": {
+            "reflect-metadata": "^0.1.12"
+          }
+        }
       }
     },
     "r2-navigator-js": {
@@ -6886,18 +6909,37 @@
       "resolved": "https://registry.npmjs.org/r2-navigator-js/-/r2-navigator-js-1.0.0-alpha.5.tgz",
       "integrity": "sha512-pELhDV2WVzzmPXfkNMoPqcqgd9X9XzbBJi0RPcda5373sMY7YBXIRMth1Y4j0c50U7uYzsV2Egh/j2X+H+NA7w==",
       "requires": {
-        "css-element-queries": "github:marcj/css-element-queries#e1f3e0475a52b830d4b6490abd7b2984fbc8553f",
         "debounce": "^1.2.0",
         "debug": "^4.0.1",
         "express": "^4.16.3",
         "moment": "^2.22.2",
         "r2-lcp-js": "^1.0.0-alpha.4",
         "r2-streamer-js": "^1.0.0-alpha.4",
-        "resize-sensor": "github:normanzb/resize-sensor#4cbb8ec7f454c985f2b89672afb46d8cbcf89497",
-        "ta-json": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
         "tslib": "^1.9.3",
         "urijs": "^1.19.1",
         "uuid": "^3.3.2"
+      },
+      "dependencies": {
+        "css-element-queries": {
+          "version": "github:marcj/css-element-queries#e1f3e0475a52b830d4b6490abd7b2984fbc8553f",
+          "from": "github:marcj/css-element-queries#e1f3e0475a52b830d4b6490abd7b2984fbc8553f"
+        },
+        "reflect-metadata": {
+          "version": "0.1.12",
+          "resolved": "https://registry.npmjs.org/reflect-metadata/-/reflect-metadata-0.1.12.tgz",
+          "integrity": "sha512-n+IyV+nGz3+0q3/Yf1ra12KpCyi001bi4XFxSjbiWWjfqb52iTTtpGXmCCAOWWIAn9KEuFZKGqBERHmrtScZ3A=="
+        },
+        "resize-sensor": {
+          "version": "github:normanzb/resize-sensor#4cbb8ec7f454c985f2b89672afb46d8cbcf89497",
+          "from": "github:normanzb/resize-sensor#4cbb8ec7f454c985f2b89672afb46d8cbcf89497"
+        },
+        "ta-json": {
+          "version": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
+          "from": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
+          "requires": {
+            "reflect-metadata": "^0.1.12"
+          }
+        }
       }
     },
     "r2-opds-js": {
@@ -6908,9 +6950,22 @@
         "debug": "^4.0.1",
         "r2-shared-js": "^1.0.0-alpha.4",
         "r2-utils-js": "^1.0.0-alpha.4",
-        "ta-json": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
         "tslib": "^1.9.3",
         "xmldom": "^0.1.27"
+      },
+      "dependencies": {
+        "reflect-metadata": {
+          "version": "0.1.12",
+          "resolved": "https://registry.npmjs.org/reflect-metadata/-/reflect-metadata-0.1.12.tgz",
+          "integrity": "sha512-n+IyV+nGz3+0q3/Yf1ra12KpCyi001bi4XFxSjbiWWjfqb52iTTtpGXmCCAOWWIAn9KEuFZKGqBERHmrtScZ3A=="
+        },
+        "ta-json": {
+          "version": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
+          "from": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
+          "requires": {
+            "reflect-metadata": "^0.1.12"
+          }
+        }
       }
     },
     "r2-shared-js": {
@@ -6925,10 +6980,23 @@
         "r2-lcp-js": "^1.0.0-alpha.4",
         "r2-utils-js": "^1.0.0-alpha.4",
         "slugify": "^1.3.1",
-        "ta-json": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
         "tslib": "^1.9.3",
         "xmldom": "^0.1.27",
         "xpath": "^0.0.27"
+      },
+      "dependencies": {
+        "reflect-metadata": {
+          "version": "0.1.12",
+          "resolved": "https://registry.npmjs.org/reflect-metadata/-/reflect-metadata-0.1.12.tgz",
+          "integrity": "sha512-n+IyV+nGz3+0q3/Yf1ra12KpCyi001bi4XFxSjbiWWjfqb52iTTtpGXmCCAOWWIAn9KEuFZKGqBERHmrtScZ3A=="
+        },
+        "ta-json": {
+          "version": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
+          "from": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
+          "requires": {
+            "reflect-metadata": "^0.1.12"
+          }
+        }
       }
     },
     "r2-streamer-js": {
@@ -6953,7 +7021,6 @@
         "request": "^2.88.0",
         "request-promise-native": "^1.0.5",
         "selfsigned": "^1.10.3",
-        "ta-json": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
         "tmp": "^0.0.33",
         "tslib": "^1.9.3",
         "uuid": "^3.3.2",
@@ -6980,6 +7047,18 @@
           "version": "0.4.1",
           "resolved": "https://registry.npmjs.org/json-schema-traverse/-/json-schema-traverse-0.4.1.tgz",
           "integrity": "sha512-xbbCH5dCYU5T8LcEhhuh7HJ88HXuW3qsI3Y0zOZFKfZEHcpWiHU/Jxzk629Brsab/mMiHQti9wMP+845RPe3Vg=="
+        },
+        "reflect-metadata": {
+          "version": "0.1.12",
+          "resolved": "https://registry.npmjs.org/reflect-metadata/-/reflect-metadata-0.1.12.tgz",
+          "integrity": "sha512-n+IyV+nGz3+0q3/Yf1ra12KpCyi001bi4XFxSjbiWWjfqb52iTTtpGXmCCAOWWIAn9KEuFZKGqBERHmrtScZ3A=="
+        },
+        "ta-json": {
+          "version": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
+          "from": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
+          "requires": {
+            "reflect-metadata": "^0.1.12"
+          }
         }
       }
     },
@@ -6994,12 +7073,27 @@
         "reflect-metadata": "^0.1.12",
         "request": "^2.88.0",
         "request-promise-native": "^1.0.5",
-        "ta-json": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
         "tslib": "^1.9.3",
         "unzipper": "^0.9.3",
         "xpath": "^0.0.27",
         "yauzl": "^2.10.0",
         "yazl": "^2.4.3"
+      },
+      "dependencies": {
+        "ta-json": {
+          "version": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
+          "from": "github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6",
+          "requires": {
+            "reflect-metadata": "^0.1.12"
+          },
+          "dependencies": {
+            "reflect-metadata": {
+              "version": "0.1.12",
+              "resolved": "https://registry.npmjs.org/reflect-metadata/-/reflect-metadata-0.1.12.tgz",
+              "integrity": "sha512-n+IyV+nGz3+0q3/Yf1ra12KpCyi001bi4XFxSjbiWWjfqb52iTTtpGXmCCAOWWIAn9KEuFZKGqBERHmrtScZ3A=="
+            }
+          }
+        }
       }
     },
     "randomatic": {
@@ -7381,10 +7475,6 @@
         "editions": "^1.1.1"
       }
     },
-    "resize-sensor": {
-      "version": "github:normanzb/resize-sensor#4cbb8ec7f454c985f2b89672afb46d8cbcf89497",
-      "from": "github:normanzb/resize-sensor#master"
-    },
     "resolve": {
       "version": "1.8.1",
       "resolved": "https://registry.npmjs.org/resolve/-/resolve-1.8.1.tgz",
@@ -8426,8 +8516,13 @@
       "resolved": "https://registry.npmjs.org/system-font-families/-/system-font-families-0.5.0.tgz",
       "integrity": "sha1-uqedR9C5ryG1AmiU/fMroekwIpU=",
       "requires": {
-        "babel-polyfill": "^6.23.0",
-        "ttfinfo": "git+https://github.com/rBurgett/ttfinfo.git#f00e43e2a6d4c8a12a677df20b7804492d50863c"
+        "babel-polyfill": "^6.23.0"
+      },
+      "dependencies": {
+        "ttfinfo": {
+          "version": "git+https://github.com/rBurgett/ttfinfo.git#f00e43e2a6d4c8a12a677df20b7804492d50863c",
+          "from": "git+https://github.com/rBurgett/ttfinfo.git#f00e43e2a6d4c8a12a677df20b7804492d50863c"
+        }
       }
     },
     "ta-json": {
@@ -8680,10 +8775,6 @@
         "tslib": "^1.8.1"
       }
     },
-    "ttfinfo": {
-      "version": "git+https://github.com/rBurgett/ttfinfo.git#f00e43e2a6d4c8a12a677df20b7804492d50863c",
-      "from": "git+https://github.com/rBurgett/ttfinfo.git"
-    },
     "tty-browserify": {
       "version": "0.0.1",
       "resolved": "https://registry.npmjs.org/tty-browserify/-/tty-browserify-0.0.1.tgz",
danielweck commented 5 years ago

Bad news, since updating to MacOS Mojave with latest LTS NodeJS and NPM, I observe the same bogus npm install which creates duplicated packages (npm ci works fine). We badly need the migration of git(hub) NPM dependencies, I am putting a high priority on this task. CC @llemeurfr @clebeaupin @JayPanoz

JayPanoz commented 5 years ago

So that means publishing ReadiumCSS on npm, right?

danielweck commented 5 years ago

Typical error due to duplicated ta-json, reflect-metadata (inccorect LCP model deserialization from license.lcpl JSON):

TypeError: lcpl.init is not a function

  r2:streamer#http/server TypeError: lcpl.init is not a function
  r2:streamer#http/server     at Object.EpubParsePromise (/Users/danielweck/Code/r2-testapp-js/node_modules/r2-shared-js/dist/es8-es2017/src/parser/epub.js:126:14)
  r2:streamer#http/server     at <anonymous>
  r2:streamer#http/server     at process._tickCallback (internal/process/next_tick.js:188:7) +18s
  r2:testapp#electron/main/index TypeError: lcpl.init is not a function
  r2:testapp#electron/main/index     at Object.EpubParsePromise (/Users/danielweck/Code/r2-testapp-js/node_modules/r2-shared-js/dist/es8-es2017/src/parser/epub.js:126:14)
  r2:testapp#electron/main/index     at <anonymous>
  r2:testapp#electron/main/index     at process._tickCallback (internal/process/next_tick.js:188:7) +117ms
danielweck commented 5 years ago

Other possible web console error: Cannot find module 'ttfinfo' (dependency of system-font-families)

Uncaught Error: Cannot find module 'ttfinfo'
    at Module._resolveFilename (module.js:485:15)
    at Function.Module._resolveFilename (/Users/danielweck/Code/r2-testapp-js/node_modules/electron/dist/Electron.app/Contents/Resources/electron.asar/common/reset-search-paths.js:35:12)
    at Function.Module._load (module.js:437:25)
    at Module.require (module.js:513:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/danielweck/Code/r2-testapp-js/node_modules/system-font-families/dist/main.js:45:16)
    at Object.<anonymous> (/Users/danielweck/Code/r2-testapp-js/node_modules/system-font-families/dist/main.js:289:3)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
danielweck commented 5 years ago

@JayPanoz ReadiumCSS not yet integrated as a NPM dependency in the build system of r2-navigator-js, but in the future to be on the safe side: better publish properly to the official NPM registry.

JayPanoz commented 5 years ago

OK well noted (sorry, an “eventually” went missing in my original message).

danielweck commented 5 years ago

The fix / workaround seems to work (MacOS), testing the R2-JS utils and lcp packages now on Linux.

See commits: https://github.com/readium/r2-utils-js/commit/0c0714e99d89bb225f0af4d43dc07d518115d0af https://github.com/readium/r2-lcp-js/commit/50747b4d78d30b4518787ee065f3b14cd5586f60

In a nutshell: still NPM packages hosted at GitHub (not proper NPM publish with new forked package name), but via Git URL and semver tag lookup.

danielweck commented 5 years ago

Damn, NPM install still buggy! :(

npm install in r2-utils-js works just fine, i.e. no local modifications of package-lock.json, when starting from scratch (rm -rf node_modules && rm package-lock.json && npm install) or with the default NPM lockfile from the source tree (rm -rf node_modules && npm install).

However, npm install in r2-lcp-js (which depends on r2-utils-js via regular NPM semver) modifies the default package-lock.json by moving ta-json (GitHub NPM dependency obtained via semver as tag version) from top-level only in node_modules (shared by all packages that require it), to top-level AND inside r2-utils-js => thus the duplicated package!

Technically-speaking, npm install changes the package-lock.json of r2-lcp-js by moving r2-utils-js > requires > ta-json to r2-utils-js > dependencies > ta-json ... which creates a separate ta-json folder inside node_modules/r2-utils-js/node_modules/, in addition to the already-existing one in node_modules/ (top-level).

Okay so I am afraid I must publish a fork of ta-json (and maybe other packages) to NPM, as a renamed package :( I would have preferred to avoid this, but so be it.

danielweck commented 5 years ago

Note that the above NPM bug is consistently reproducible with NodeJS 8.12 and NPM 6.4.1, on MacOS, Linux and Windows. Simply run the following commands after checking-out a clean r2-utils-js (at release tag v1.0.0-alpha.5), and to ensure starting from a really clean slate: rm -rf node_modules && git status && git diff. Then npm install && git diff (this should show the modified package-lock.json). Curiously, if we rm package-lock.json before invoking npm install, then package-lock.json is unchanged (git diff reports nothing).

danielweck commented 5 years ago

No comments ...

https://github.com/npm/npm/issues

https://npm.community/c/bugs

JayPanoz commented 5 years ago

Yeah I personally rolled back last week because of awful npm issues with v6.4.1 as they severely impacted my work e.g. constantly having to clean the cache even on a brand new install, global install taking ages to resolve, etc. – I also “limited” some private repos to v5.6.0 for the time being.

danielweck commented 5 years ago

I found these related open NPM issues: https://github.com/npm/npm/issues/3081 https://github.com/npm/npm/issues/20026 https://github.com/npm/npm/issues/21203 (there are several other critical issues related to Git dependencies, package duplicates, unwarranted removals, etc.) https://npm.community/t/npm-install-creates-incorrect-package-lock-json-if-package-lock-json-is-present/1080

llemeurfr commented 5 years ago

and ... This repository is moving to: https://github.com/npm/cli with https://github.com/npm/cli/pulls = 17

danielweck commented 5 years ago

@JayPanoz sudo npm cache clean --force FTW :)

danielweck commented 5 years ago

There is also this Continuous Integration CLI tool written by an NPM lead dev: https://github.com/zkat/cipm https://www.npmjs.com/package/cipm ...so instead of NPM (install or ci), we can use yarn, cipm, pnpm ( https://github.com/pnpm/pnpm ), etc. ;o)

danielweck commented 5 years ago

The migration to a new package name ta-json-x (from ta-json) officially published at NPM seems to work (as a replacement for the ad-hoc same-name GitHub semver dependency). Unfortunately this requires code changes, see typical diff: https://github.com/readium/r2-lcp-js/commit/14e645a2eaebd55083d7d6974311bd39e69b42f1 (more to come, as I modify every r2-xxx-js package ...)

danielweck commented 5 years ago

Okay so the problem is fixed for ta-json, which is good as this affected most r2-xxx-js components.

However, r2-navigator-js still references the css-element-queries and resize-sensor dependencies via GitHub instead of NPM, so the npm install bug still occurs. A simple user/developer-side workaround consists in invoking npm install a second time. Then package-lock.json is restored, and the node_modules hierarchy is correct again. One undesirable side effect with this hacky developer workflow is when there is a time-consuming postinstall script, such as in readium-desktop.

Now, even if we fix create proper NPM packages for css-element-queries and resize-sensor (to replace their current GitHub references), there will still be indirect cases such as ttfinfo which is required by system-font-families (in r2-test-app).

I'm looking into our options...

danielweck commented 5 years ago

Related issue: https://github.com/readium/readium-desktop/issues/190

danielweck commented 5 years ago

What is left of this issue is not actionable (i.e. NPM bugs in some versions). Let's close this issue for now.