Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NPM update #161

Merged
merged 17 commits into from
Sep 5, 2018
Merged

NPM update #161

merged 17 commits into from
Sep 5, 2018

Conversation

danielweck
Copy link
Member

  • NPM lockfile now aligned with r2-xxx-js packages.
  • Removed r2-testapp-js dependency (not needed in readium-electron).
  • Minor: added package.json script "package:pack-only" for electron-builder (a separate branch will be created later, for the customized ASAR build process)

Please merge this as soon as possible to avoid future branch conflicts :)
(if you approve, of course)

@danielweck
Copy link
Member Author

I am getting a build failure on Linux (fine on MacOS) ... investigating :(

@danielweck
Copy link
Member Author

Linux failure (TypeScript compiler / WebPack loader):
https://travis-ci.org/readium/readium-desktop/builds/393987949#L1461

Works fine on MacOS, so I compared the contents of the node_modules in particular the r2-xxx-js packages, and they seem duplicated in Linux (whereas they are correctly organised in MacOS). Same Node and NPM versions. I will try downgrading NPM, as this is usually the culprit.

@danielweck
Copy link
Member Author

Downgrading from NPM 6.* to 5.* seems to fix this. If this is confirmed, I am going to adjust the TravisCI YAML script to ensure we can build PRs...

@danielweck
Copy link
Member Author

Just as I suspected, npm ci works on 6.* when used instead of npm install.

@danielweck
Copy link
Member Author

Travis CI now passes :)
(phew!)

@danielweck
Copy link
Member Author

Note that react-dropzone version 4.2.12 introduces a regression bug (invisible publication library, yet each cover item is in the DOM), so we stick to 4.2.11.

@danielweck
Copy link
Member Author

Note that the react-dropzone version 4.2.12+ regression bug still exists (invisible publication library, yet each cover item is in the DOM), so we stick to 4.2.11 (latest is 4.3.0).
See:
react-dropzone/react-dropzone#631

@leomoty
Copy link

leomoty commented Aug 14, 2018

react-tap-event-plugin is not compatible with react >= 16.4, nor they plan on supporting it. Found this while I was looking how to compile the project itself.

zilverline/react-tap-event-plugin#121

@leomoty
Copy link

leomoty commented Aug 14, 2018

Also, should be a simple fix:

npm WARN deprecated [email protected]: You can now upgrade to @material-ui/core

@danielweck
Copy link
Member Author

Thank you for the heads-up about "react-tap-event-plugin". As people have stated in the issue tracker ( zilverline/react-tap-event-plugin#121 ): "this plugin was never supported officially and relies on private APIs that can change in any patch", and "I think it's time to end this plugin" :)
More importantly, there will soon be major changes to the readium-desktop UI (as per the plan to redesign for better visuals and accessibility) so I wouldn't worry too much about the current alpha UI state.

@danielweck
Copy link
Member Author

Regarding @material-ui/core: just to clarify, the purpose of this PR is to ensure that the r2-xxx-js dependencies are up to date (which sometimes requires code changes in readium-desktop), and also to make sure that common / shared dependencies with readium-desktop are aligned. This way, the package-lock.json file is consistent across the board, and the dependencies expressed in package.json explicitly reference their actual semver (current node_modules packages as per the NPM lockfile).

Note that I use the NCU utility to check for the latest available versions (minor and major updates). The goal is to explicitly include minor changes (to stay in sync with security updates, bug fixes, improvements, etc.), whilst filtering-out known breaking API updates. In the latter case, the 4.x.x semver notation is used instead of ^4.0.0 (for example) which gives developers a strong visual cue that some dependencies break the build (and it makes it easy to use NCU with its "exclude" flag, e.g. ncu -a -u -x electron,webpack,webpack-dev-server,etc).

So, thank you for the heads-up about @material-ui/core, but I think it is fine for now as the UI redesign will probably shake things quite a bit (including the Material deps). Cheers! :)

@leomoty
Copy link

leomoty commented Aug 14, 2018

@danielweck I am not really worried about the UI itself, I am trying to build the LCP stack, but so far I am not sure whether my .lcpl license file works or not. I tried building readium-desktop from source without success. The alpha binary works, but I cant seem to open the epub, nor does it echo any error that could help.

@danielweck
Copy link
Member Author

@leomoty here is a copy-paste of the error you mentioned here #162 (comment) :

  ERROR in [at-loader] ./src/renderer/components/ReaderApp.tsx:295:33 
        TS2345: Argument of type '(docHref: string, docSelector: string) => void' is not assignable to parameter of type '(docHref: string, locator: IEventPayload_R2_EVENT_READING_LOCATION) => void'.
      Types of parameters 'docSelector' and 'locator' are incompatible.
        Type 'IEventPayload_R2_EVENT_READING_LOCATION' is not assignable to type 'string'.
    
    ERROR in [at-loader] ./src/renderer/components/ReaderApp.tsx:671:13 
        TS2345: Argument of type 'string' is not assignable to parameter of type 'IEventPayload_R2_EVENT_READING_LOCATION'.

Are you sure you have a clean Git local copy of the branch? (please git pull && git status just to make sure everything looks good)

@leomoty
Copy link

leomoty commented Aug 14, 2018

git pull
Already up to date.
Current branch feature/npm-update-packs is up to date.

git status
On branch feature/npm-update-packs
Your branch is up to date with 'origin/feature/npm-update-packs'.

nothing to commit, working tree clean

@leomoty
Copy link

leomoty commented Aug 14, 2018

Trying to clone again on Windows

L:\>npm --version
6.1.0

L:\>node --version
v10.6.0

Will report back after npm install runs.

@leomoty
Copy link

leomoty commented Aug 14, 2018

npm install is so much better:

npm WARN optional SKIPPING OPTIONAL DEPENDENCY: [email protected] (node_modules\fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for [email protected]: wanted {"os":"darwin","arch":"any"} (current: {"os":"win32","arch":"x64"})

added 1824 packages from 1446 contributors and audited 19709 packages in 417.952s
found 0 vulnerabilities

Errors have changed:

[2] ERROR in [at-loader] ./src/main/redux/sagas/lcp.ts:74:21
[2]     TS2345: Argument of type 'import(".../readium-desktop/node_modules/r2-streamer-js/dist/es6-es2015/src/http/server").Server' is not assignable to parameter of type 'import(".../readium-desktop/node_modules/r2-navigator-js/node_modules/r2-streamer-js/dist/es6-es2015/src/http/server").Server'.
[2]
[2] ERROR in [at-loader] ./src/main/redux/sagas/lcp.ts:135:21
[2]     TS2345: Argument of type 'import(".../readium-desktop/node_modules/r2-streamer-js/dist/es6-es2015/src/http/server").Server' is not assignable to parameter of type 'import(".../readium-desktop/node_modules/r2-navigator-js/node_modules/r2-streamer-js/dist/es6-es2015/src/http/server").Server'.
[2]
[2] ERROR in [at-loader] ./src/main/streamer.ts:37:20
[2]     TS2345: Argument of type 'import(".../readium-desktop/node_modules/r2-streamer-js/dist/es6-es2015/src/http/server").Server' is not assignable to parameter of type 'import(".../readium-desktop/node_modules/r2-navigator-js/node_modules/r2-streamer-js/dist/es6-es2015/src/http/server").Server'.
[2]
[2] ERROR in [at-loader] ./src/main/streamer.ts:51:17
[2]     TS2345: Argument of type 'import(".../readium-desktop/node_modules/r2-streamer-js/dist/es6-es2015/src/http/server").Server' is not assignable to parameter of type 'import(".../readium-desktop/node_modules/r2-navigator-js/node_modules/r2-streamer-js/dist/es6-es2015/src/http/server").Server'.
[2]   Types have separate declarations of a private property 'publications'.
[2]
[2] ERROR in [at-loader] ./src/renderer/components/ReaderApp.tsx:679:13
[2]     TS2345: Argument of type 'import(".../readium-desktop/node_modules/r2-shared-js/dist/es6-es2015/src/models/publication").Publication' is not assignable to parameter of type 'import(".../readium-desktop/node_modules/r2-navigator-js/node_modules/r2-shared-js/dist/es6-es2015/src/models/publication").Publication'.
[2]   Types have separate declarations of a private property 'Internal'.
[2]
[2] ERROR in [at-loader] ./src/utils/lcp.ts:115:17
[2]     TS2345: Argument of type 'import(".../readium-desktop/node_modules/r2-shared-js/node_modules/r2-lcp-js/dist/es6-es2015/src/parser/epub/lcp").LCP' is not assignable to parameter of type 'import(".../readium-desktop/node_modules/r2-lcp-js/dist/es6-es2015/src/parser/epub/lcp").LCP'.
[2]   Types have separate declarations of a private property '_usesNativeNodePlugin'.

I just omitted my absolute path, everything else is verbatim.

@danielweck
Copy link
Member Author

Note that I've had build failures in the past with NodeJS version 10+, so nowadays I stick to LTS version 8 on Linux, MacOS and Windows. NPM however is latest 6.3.0 (via npm --global update npm after installing the official NodeJS distribution). The Electron 1.x.x runtime is actually NodeJS 8 so this is another reason why I like to run all my r2-streamer-js tests on the same LTS version.

@danielweck
Copy link
Member Author

Note that the r2-xxx-js package.json dependencies in master, develop and this PR branch feature/npm-update-packs all point to the same Git "commit-ish" reference (i.e. develop named branch/tag inside special "dist" repositories which are used instead of npmjs.com to publish builds):
https://github.com/readium/readium-desktop/blob/master/package.json#L127
https://github.com/readium/readium-desktop/blob/develop/package.json#L134
https://github.com/readium/readium-desktop/blob/feature/npm-update-packs/package.json#L136
...but the package-lock.json actual revision should take precedence when invoking npm install or npm ci. Seems to work fine with NPM 6+, and used to work with v5.

@danielweck
Copy link
Member Author

It is worth noting that TravisCI (Linux) successfully builds this PR branch feature/npm-update-packs using NodeJS LTS v8.11.3 and the latest NPM v6.3.0.

See:
https://travis-ci.org/readium/readium-desktop/builds/415887205

@leomoty
Copy link

leomoty commented Aug 14, 2018

I am currently running npm install both with LTS node and latest NPM.

@leomoty
Copy link

leomoty commented Aug 14, 2018

Still getting the same error, however, running npm list I noticed this:

image

image

These are not necessarily all errors, but I am under impression all yours are marked as ìnvalid`.

@leomoty
Copy link

leomoty commented Aug 14, 2018

It doesn't seem to be first level packages tho:

npm list | Select-String -Pattern invalid

| +-- [email protected] invalid (github:edrlab/r2-utils-js-dist#812b725435a898622831724b698f41a238ac2539)
| +-- [email protected] invalid (github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6)
| +-- [email protected] invalid (github:marcj/css-element-queries#e1f3e0475a52b830d4b6490abd7b2984fbc8553f)
| +-- [email protected] invalid (github:edrlab/r2-lcp-js-dist#0ca8931c2f5b0c4d290d9bba1542e27188af0bfe)
| +-- [email protected] invalid (github:edrlab/r2-opds-js-dist#005396ae530b8a3595cce2e32be4b676dc4acb97)
| +-- [email protected] invalid (github:edrlab/r2-shared-js-dist#6e6c2db73524190b31d56b979b740812113269e6)
| +-- [email protected] invalid (github:edrlab/r2-streamer-js-dist#ea8fcc1e8c46e7febdce6ab2dc570cd31b09b173)
| +-- [email protected] invalid (github:edrlab/r2-utils-js-dist#812b725435a898622831724b698f41a238ac2539)
| +-- [email protected] invalid (github:normanzb/resize-sensor#4cbb8ec7f454c985f2b89672afb46d8cbcf89497)
| +-- [email protected] invalid (github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6)
| +-- [email protected] invalid (github:edrlab/r2-utils-js-dist#812b725435a898622831724b698f41a238ac2539)
| +-- [email protected] invalid (github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6)
| +-- [email protected] invalid (github:edrlab/r2-lcp-js-dist#0ca8931c2f5b0c4d290d9bba1542e27188af0bfe)
| +-- [email protected] invalid (github:edrlab/r2-opds-js-dist#005396ae530b8a3595cce2e32be4b676dc4acb97)
| +-- [email protected] invalid (github:edrlab/r2-utils-js-dist#812b725435a898622831724b698f41a238ac2539)
| +-- [email protected] invalid (github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6)
| +-- [email protected] invalid (github:edrlab/r2-lcp-js-dist#0ca8931c2f5b0c4d290d9bba1542e27188af0bfe)
| +-- [email protected] invalid (github:edrlab/r2-opds-js-dist#005396ae530b8a3595cce2e32be4b676dc4acb97)
| +-- [email protected] invalid (github:edrlab/r2-shared-js-dist#6e6c2db73524190b31d56b979b740812113269e6)
| +-- [email protected] invalid (github:edrlab/r2-utils-js-dist#812b725435a898622831724b698f41a238ac2539)
| +-- [email protected] invalid (github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6)
| +-- [email protected] invalid (github:danielweck/ta-json#9938af9c192ca36aa600f2ce14eb5735aa1a50e6)

@leomoty
Copy link

leomoty commented Aug 14, 2018

Okay, I was able to successfully load it:

 npm update

+ [email protected]
+ [email protected]
+ [email protected]
+ [email protected]
+ [email protected]
+ [email protected]
+ [email protected]
+ [email protected]
removed 70 packages, updated 9 packages, moved 2 packages and audited 19709 packages in 32.054s
found 0 vulnerabilities

It compiled now. I have no clue what is the issue, but running npm update fixed it.

@danielweck
Copy link
Member Author

Makes sense. Not.

@leomoty
Copy link

leomoty commented Aug 14, 2018

Also, I know for sure it is my issue now, I guess:

[2] ## downloadContent err

I think some config from config.yaml at lcp-server README could be outdated or something, but I am not 100% sure how to define license.links.publication. I am currently testing with the Frontend app, by the way.

@danielweck
Copy link
Member Author

Thank you for your debunking of this NPM issue. So it is now quite clear that GitHub dependencies ( as officially supported by NPM https://docs.npmjs.com/cli/install ) are problematic, requiring an extra npm update PACKAGE_NAME for each culprit (or a more general npm update which could be problematic because this would affect other non-GitHub packages, and potentially modify the package-lock.json)

@leomoty
Copy link

leomoty commented Aug 14, 2018

@danielweck this is not the first time I have issues with npm, which is why I guess people suggests yarn? I can't say for sure, never used it myself. I'll try to go back to make the readium-desktop read my encrypted epub file. Thank you!

@danielweck
Copy link
Member Author

@leomoty can you please log an issue at https://github.com/edrlab/r2-lcp-js/
(I will be happy to discuss LCP integration there)

@leomoty
Copy link

leomoty commented Aug 14, 2018

Done, readium/r2-lcp-js#2

@danielweck
Copy link
Member Author

I filed a separate issue regarding Material-UI:
#164

@clebeaupin clebeaupin merged commit dfdb4f4 into develop Sep 5, 2018
@danielweck danielweck deleted the feature/npm-update-packs branch September 19, 2018 08:46
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.

3 participants