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

Move js dependencies required for storybook into their own workspace #10091

Open
4 tasks
eugene-manuilov opened this issue Jan 21, 2025 · 6 comments
Open
4 tasks
Assignees
Labels
P2 Low priority Type: Infrastructure Engineering infrastructure & tooling

Comments

@eugene-manuilov
Copy link
Collaborator

eugene-manuilov commented Jan 21, 2025

Feature Description


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The package.json file is updated to have storybook in the workspaces array.
  • JavaScript dependencies that are needed for storybook to function properly are moved to the new workspace.
  • GitHub actions for storybook are updated accordingly to continue running it for all PRs.
  • The storybook config property is moved from the main package.json file to the workspace package.json.
    "storybook_wordpress_version": "5.5.1",

Implementation Brief

  • Update the root package.json file to include "storybook" in its workspaces array.
  • Create a new package.json file in the storybook folder using npm init -w ./storybook.
    • Copy the scripts for the storybook but not for building the storybook to this new package.json.
    • Fix any paths and ensure the commands work correctly after the refactor.*
  • Scan all dependencies that we have in the main package.json file and move all dependencies that are used in storybook to its own package by uninstalling them in the main package.json file and installing with -w ./storybook argument to specify the correct workspace.
  • Update the package.json file to proxy storybook commands to the correct workspace using the npm run ... -w ./storybook approach.

Test Coverage

  • N/A

QA Brief

Changelog entry

@eugene-manuilov eugene-manuilov added P2 Low priority Type: Infrastructure Engineering infrastructure & tooling labels Jan 21, 2025
@hussain-t hussain-t self-assigned this Jan 22, 2025
@hussain-t hussain-t removed their assignment Jan 23, 2025
@techanvil techanvil self-assigned this Jan 24, 2025
@techanvil
Copy link
Collaborator

techanvil commented Jan 24, 2025

Hi @hussain-t, thanks for drafting this IB. It's mostly looking good, but I am a bit unsure about this point (presumably you mean copying the scripts for running Storybook, but that's not what I am unsure about):

Copy the scripts for the storybook but not for building the storybook to this new package.json.

Considering the build:storybook NPM script runs build-storybook, which is provided by the @storybook/react package, and also specifies the .storybook folder as its config location, if we keep build:storybook at the root level we'll need to keep the Storybook package(s) installed at the root level too, which partially defeats the point of moving Storybook to a workspace. We might also need to retain additional root level packages depending on how the config resolves things.

I think we should move all Storybook scripts into the workspace package.json. We should probably still output to dist in the repo root for now though, to keep the migration easy to manage, although at some point it would be nice to move all the workspaces to separate folders under a root workspace folder and we might want to output to a separate dist folder at that point...

Also, regarding this point:

Scan all dependencies that we have in the main package.json file and move all dependencies that are used in storybook to its own package by uninstalling them in the main package.json file and installing with -w ./.storybook argument to specify the correct workspace.

That mostly works, but we need to take care to ensure that shared dependencies (e.g. @wordpress/element & the underlying react) are managed correctly with matching versions. We need to check that common dependencies of the packages we install for Storybook match with those that are installed at the root level, and we might need to install packages like @wordpress/element at the workspace level while leaving it in the root package.json.

Alternatively, maybe we can simply leave our @wordpress/* packages, and so forth, in the root package.json and not specify them in the workspace version. I'm not sure if we'd run into any problems taking that approach... 🤔 This would be worth exploring further at the IB stage to make sure we identify the correct way forward.

@techanvil techanvil assigned hussain-t and unassigned techanvil Jan 24, 2025
@techanvil
Copy link
Collaborator

techanvil commented Jan 24, 2025

That mostly works, but we need to take care to ensure that shared dependencies (e.g. @wordpress/element & the underlying react) are managed correctly with matching versions. We need to check that common dependencies of the packages we install for Storybook match with those that are installed at the root level, and we might need to install packages like @wordpress/element at the workspace level while leaving it in the root package.json.

Alternatively, maybe we can simply leave our @wordpress/* packages, and so forth, in the root package.json and not specify them in the workspace version. I'm not sure if we'd run into any problems taking that approach... 🤔 This would be worth exploring further at the IB stage to make sure we identify the correct way forward.

Actually, thinking about it we're going to run into the same sort of issue in #10092 - essentially, we need to make sure our versions of packages used by the files in assets/js match for the storybook and assets workspaces, or that we only specify the dependencies in a single location.

Working on this issue and #10092 in parallel could lead to conflicts, I think it would make sense to do one after the other. I've gone ahead and made this issue a dependency for #10092.

@eugene-manuilov
Copy link
Collaborator Author

@hussain-t, i have updated AC to move storybook config as well. Plus, pay attention that .storybook is going to be renamed to storybook in #10090 because a workspace name can't start with a dot.

@benbowler
Copy link
Collaborator

Note that once #10113 is merged this ticket can move puppeteer and puppeteer-core to the workspace package.json files.

@techanvil
Copy link
Collaborator

Note that once #10113 is merged this ticket can move puppeteer and puppeteer-core to the workspace package.json files.

Thanks @benbowler, did you mean to link to a different issue though?

@benbowler
Copy link
Collaborator

benbowler commented Jan 28, 2025

@techanvil similar looking number but I meant: #10013

@hussain-t hussain-t removed their assignment Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Low priority Type: Infrastructure Engineering infrastructure & tooling
Projects
None yet
Development

No branches or pull requests

4 participants