search

applitools / eyes-storybook

Applitools SDK for Storybook. This repository is deprecated. It has moved to https://github.com/applitools/eyes.sdk.javascript1/tree/master/packages/eyes-storybook
Other
13 stars 6 forks source link

Adding storybook/core to dependencies #25

Closed skattabrain closed 5 years ago

skattabrain commented 5 years ago

src/startStorybookServer.js requires storybook/core explicitly but it's not declared as a dependency in the package.json. This PR addresses this issue.

The package-lock diff was surprisingly large. It was generated with NPM version 6, I'd be happy to update based upon your requirements.

A little context, using this package with PNPM we received this error...

Using @applitools/eyes-storybook version 2.7.25. 

ENOENT: no such file or directory, open '/my-project/node_modules/@storybook/core/package.json'

This looks like a classic phantom dependency issue - https://rushjs.io/pages/advanced/phantom_deps/#phantom-dependencies

skattabrain commented 5 years ago

Like to also note, I've also tried the latest at this time - 2.7.36 and it's an issue there as well.

danielschwartz85 commented 5 years ago

@skattabrain Thanks for this issue. We are planning to remove this dependency. I Will update here once it's done.

we don't want to add this to our package json since it's not a direct dependency i.e. the user should have this package (we could have added it as peer dependency but not sure if that would help in pnpm case anyhow) regardless we don't have to get the version, so we will remove this dependency entirely..

kaidjohnson commented 5 years ago

Thanks for this project, btw. I 100% agree that removing the dependency altogether is the way to go -- fewer dependencies is almost always an improvement.

we don't want to add this to our package json since it's not a direct dependency

Here, I disagree. Your code explicitly calls require('@storybook/core/package.json');, so your package.json (eg dependency manifest) should declare it as a dependency, even if it is as a peer dependency (although I still think an explicit dependency is appropriate, as npx @applitools/eyes-storybook --help, for example, fails with same error as posted by @skattabrain above). By not declaring it at all, it becomes a phantom dependency, and it's only because npm installs packages flatly that resolution can even work here, but that's an argument for another day.

A declaration as a peerDependency would cue the developer to install @storybook/core directly to their project, which does fix the issue, even for pnpm. The reason that pnpm is unhappy is that users don't typically install @storybook/core directly; they install a flavor of storybook like @storybook/vue which depends on @storybook/core. This is where pnpm's non-flat installation (correctly) fails to resolve the module lookup -- if it's not found in folder c, check parent b, then check parent a, etc. But it can't go up and then down the folder tree, which would be required to discover @storybook/core as a dependency of @storybook/vue required by @applitools/eyes-storybook.

The rushjs team has done a great job documenting this trap here: https://rushjs.io/pages/advanced/phantom_deps and the pnpm author has a great writeup on the subject as well: https://medium.com/pnpm/flat-node-modules-is-not-the-only-way-d2e40f7296a3. They're both well worth the read!

Of course, it's all moot if there's no dependency at all :)

danielschwartz85 commented 5 years ago

Hi @kaidjohnson, You are right, when I said:

we don't want to add this to our package json since it's not a direct dependency

I meant we don't want to add it as a direct dependency (peer would be ok). Regardless thanks for the pnpm info and links, will check it out 👍

danielschwartz85 commented 5 years ago

Removed the storybook (version) dependency with 2.8.6. LMK if this still has any issues..

skattabrain commented 5 years ago

Thank you Daniel!