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

[Bug]: Try to avoid JsPackageManager.retrievePackageJSON usage #26263

Open
valentinpalkovic opened this issue Feb 29, 2024 · 3 comments
Open
Labels

Comments

@valentinpalkovic
Copy link
Contributor

valentinpalkovic commented Feb 29, 2024

Describe the bug

The JsPackageManager.retrievePackageJSON method in @storybook/core-common returns currently a package.json file which is looked up in the current working directory.

The package.json is used to figure out,

  • whether specific dependencies are installed (for example, which renderer or framework is used).
  • to add the Storybook scripts during installation or to manipulate it during automigration
  • to remove/add dependencies in the case where we want to skip installation (otherwise JSPackageManager.runAddDeps or JsPackageManager.runRemoveDeps is called, which uses the package manager CLI to remove/add dependencies)
  • to evaluate the installed Storybook version

All the mentioned use cases might run into issues when Storybook is used, initialized, or upgraded in a mono repo setup. The folder into which Storybook should be initialized or is already initialized might

  • not have a package.json, because dependencies are handled at the root (e.g. NX's one-version policy).
  • might have a package.json, but only scripts are defined because dependencies are still installed at the root.
  • The root (cwd) doesn't have a package.json, but each app does (This case is already more or less handled if the --config-dir argument is passed to the CLI (not all CLI commands support --config-dir, e.g. npx storybook add doesn't))

When adding or removing dependencies or asking for a particular dependency, we should use always the package manager's CLI commands to get this information, because we simply cannot rely on the package.json, which is defined at the root.

Acceptance criteria

  • Go through the monorepo and figure out, whether JsPackageManager.retrievePackageJSON is used in scenarios, which aren't mentioned above.
  • Replace each JsPackageManager.retrievePackageJSON usage by methods, which call the package manager's CLI directly to get all the necessary information.

Related issues

To Reproduce

No response

System

No response

Additional context

No response

@kasir-barati
Copy link

kasir-barati commented Apr 6, 2024

Any workaround for Nx for the time being? I am running into this issue when I execute nx storybook frontend. It creates a package.json which is of course uninvited. So far what I could think of was to add it to git ignore or remove it automatically after it was created;

"run:storybook": {
  "executor": "nx:run-commands",
  "options": {
    "parallel": false,
    "commands": [
      "nx storybook frontend",
      "rm ./apps/frontend/package.json"
    ]
  }
}

Which is obviously a very tough to manage solution since we have other commands too and it might also not be possible for publishable libs who need their package.json.

Suggestion

Maybe a flag in StorybookConfig.core like disablePackageJson, exactly like what we have for other things such as: disableProjectJson:

apps/frontend/.storybook/main.ts:

import type { StorybookConfig } from '@storybook/nextjs';

export default {
  // ...
  core: {
    disablePackageJson: true,
    // ...
  },
  // ...
} satisfies StorybookConfig;

Any thought @valentinpalkovic?

@valentinpalkovic
Copy link
Contributor Author

Hi @kasir-barati

Your proposed solution would work, but I would instead try to fix the root cause of the issue (detecting the correct package.json) instead of implementing a workaround. As described above, the solution isn't that straightforward and depends on a couple of things. Hopefully, I can manage to work on this soon.

@kasir-barati
Copy link

Great news @valentinpalkovic,

Thanks a lot. I am really looking forward to it and please lemme know when it is done so that I can have it in my proj since I am right now gitignoring it and it is not much of a problem but having less things to keep in mind is always a good thing.

I feel you, it is imperative to solve a problem from its root and fix those broken windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Empathy Backlog
Development

No branches or pull requests

3 participants