stencila / encoda

↔️ A format converter for Stencila documents
https://stencila.github.io/encoda/
Apache License 2.0
35 stars 9 forks source link

Install issue with patch-package #965

Closed rgieseke closed 3 years ago

rgieseke commented 3 years ago

When trying to install 0.117.4 i had to also install patch-package, otherwise i got

npm ERR! command failed
npm ERR! command sh -c patch-package
npm ERR! sh: 1: patch-package: not found

Installing encoda in a clone of this repo works without problems. Does patch-package maybe need to be moved to be a dependency, not dev-dependency?

alex-ketch commented 3 years ago

Thanks for the report @rgieseke 👍 After taking a look I don't think we need to add patch-package to dependencies, but rather change our project configuration a bit.

Right now we trigger running patch-package through NPMs postinstallscript. This gets run even when Encoda is installed as a dependency in another project. I think we can instead only run it as a prepare script, meaning it will only execute during when working on the Encoda project itself, and not when consuming it as a dependency. @nokome has more context around the need for running patch-package, so will wait for him to chime in before making any changes.

rgieseke commented 3 years ago

Thanks for the quick reply, i came across this issue, so it seems postinstall is the recommended way: https://github.com/ds300/patch-package/issues/46

There are some discussions linked in the thread though.

alex-ketch commented 3 years ago

Thanks for the quick reply, i came across this issue, so it seems postinstall is the recommended way: ds300/patch-package#46

Hmm, good to know as it might have saved me a lot of headache :) I do wonder if the issue is outdated now as NPM documentation says that prepare scripts should run under npm ci, "should" being the operating word…

rgieseke commented 3 years ago

Tried with prepare and both 'npm install' and 'npm ci' seem to have run through without any errors, no idea whether that makes a difference for any development workflows.

> @stencila/encoda@0.117.4 prepare
> patch-package

patch-package 6.4.7
Applying patches...
jsonld@5.2.0 ✔
nokome commented 3 years ago

Sorry for the late reply on this.

The reason that I added patch-package is to comment out some code from the jsonld package that is incompatible with the building a standalone binary with pkg. These warning were resulting in a hard errors at runtime:

> Warning Failed to make bytecode node16-x64 for file /snapshot/encoda/node_modules/@digitalbazaar/http-client/main.js
> Warning Failed to make bytecode node16-x64 for file /snapshot/encoda/node_modules/ky/index.js

Does patch-package maybe need to be moved to be a dependency, not dev-dependency?

Yes, I believe that is the issue, since the patching of the package needs to be done for a prod install too. I've made that fix but in the longer term I'm considering perhaps not doing the pkg-based binary builds anyway. So the need for patch-package may go away.

nokome commented 3 years ago

Seems to be fixed now.