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

Not fully compatible with yarn package manager #7

Closed
amokrushin opened this issue May 21, 2017 · 7 comments
Closed

Not fully compatible with yarn package manager #7

amokrushin opened this issue May 21, 2017 · 7 comments

Comments

@amokrushin
Copy link
Contributor

amokrushin commented May 21, 2017

yarn add exiftool-vendored
node index.js
OK
yarn add anything-else
node index.js
Error: Cannot find module 'exiftool-vendored.pl'
    at Function.Module._resolveFilename (module.js:470:15)
    at Function.Module._load (module.js:418:25)
    at Module.require (module.js:498:17)
    at require (internal/module.js:20:19)
    at Object.<anonymous> (/srv/nodejs/test-exiftool/node_modules/exiftool-vendored/dist/ExifTool.js:17:20)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)

index.js includes only import of exiftool-vendored module

Works ok only on clean install or if exiftool-vendored.pl explicitly added as a package dependency.

This does not cause much inconvenience in a case of exiftool-vendored is a direct dependency. But it is not so if there are intermediate dependencies.
For example module-a -> module-b -> exiftool-vendored. Whenever a module-a adds a new dependency, mobule-b will throw an error.

@mceachen
Copy link
Member

This is a bug in yarn's interaction with platform-dependent-modules. If you know of another way to have platform-specific dependencies, I'd appreciate any suggestions.

@amokrushin
Copy link
Contributor Author

amokrushin commented May 22, 2017

It's not a bug of yarn, it is an expected behaviour. yarn add command automatically removes all not explicitly defined modules.
You already defined an os section in exiftool-vendored.exe and exiftool-vendored.pl modules, so you should be able to remove your platform-dependent-modules dependency and replace package.json's config section with

"optionalDependencies": {
    "exiftool-vendored.pl": "10.51.0",
    "exiftool-vendored.exe": "10.51.0"
}

Package managers (npm or yarn) will not install modules which aren't compatible with the current platform os. So on a target platform only one of that modules will be installed.
But I don't know has this solution any side effect or not.

@mceachen
Copy link
Member

mceachen commented May 22, 2017

I actually used this approach in versions prior to v1.5.3, see 3690c73#diff-b9cfc7f2cdf78a7f4b91a753d10865a2.

This post highlights a defect with this approach: if the dependency fails to install, npm just reports it as a warning and carries on. This issue in yarn is somewhat related.

@amokrushin
Copy link
Contributor Author

amokrushin commented May 22, 2017

I don't understand what the problem was with that solution.

It seems working as expected with both of yarn and npm:

am@135775:/srv/nodejs/qaa1nd07/base$ yarn
yarn install v0.24.5
info No lockfile found.
[1/4] Resolving packages...
[2/4] Fetching packages...
warning qaa1nd07-dep-win32@0.1.1: The platform "linux" is incompatible with this module.
info "qaa1nd07-dep-win32@0.1.1" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/4] Linking dependencies...
[4/4] Building fresh packages...
success Saved lockfile.
Done in 0.37s.
am@135775:/srv/nodejs/qaa1nd07/base$ npm i
qaa1nd07-base@0.1.0 /srv/nodejs/qaa1nd07/base
└── qaa1nd07-dep-linux@0.1.1

npm WARN optional SKIPPING OPTIONAL DEPENDENCY: qaa1nd07-dep-win32@^0.1.1 (node_modules/qaa1nd07-dep-win32):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for qaa1nd07-dep-win32@0.1.1: wanted {"os":"win32","arch":"any"} (current: {"os":"linux","arch":"x64"})
npm WARN qaa1nd07-base@0.1.0 No description
npm WARN qaa1nd07-base@0.1.0 No repository field.

If only problew was

Making all versions of the dependency optional means that if your installation fails for a legitimate reason, it will silently skip installation and you will be missing a dependency you really need.

Then postinstall hook can verify that one was installed and throw error if not.

@mceachen
Copy link
Member

NICE! I didn't even think about doing a validation step in the postinstall. Do you want to make a PR for this, or wait for me to do it?

Thanks!

amokrushin added a commit to amokrushin/exiftool-vendored.js that referenced this issue May 23, 2017
@amokrushin
Copy link
Contributor Author

PR is ready

@mceachen
Copy link
Member

Thanks for your help. Travis now runs the builds with yarn, and I'll release this version tomorrow.

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 a pull request may close this issue.

2 participants