feat: hello, monorepo! by tkajtoch · Pull Request #7736 · elastic/eui

tkajtoch · 2024-05-06T14:19:59Z

Summary

This PR moves all EUI source files to packages/eui (including src-docs) and enables yarn workspaces to link local versions of all EUI packages - which is currently one 🤣

I recommend reviewing commit per commit due to Github's inefficiency in rendering lots of file moves.

The goal of this change is to allow the separation of EUI components into their own small packages in the (near) future. This has many benefits like independent versioning possibility, increased performance and developer experience, separation of concerns, and more.

QA

This PR should be reviewed as a major dependency upgrade due to the possibility of affecting dependency resolution.

Checkout this branch locally and cd into packages/eui
Run yarn build and ensure EUI builds with no errors
Run yarn test-unit and yarn test-cypress and ensure EUI passes all tests
Run yarn lint and ensure all lining tools work and the code passes tests
Run yarn start, wait for it to build, visit http://localhost:8030 and test the docs site works as expected

General checklist

Browser QA
- Checked in both light and dark modes
- Checked in mobile
- Checked in Chrome, Safari, Edge, and Firefox
Docs site QA
- Props have proper autodocs (using @default if default values are missing) and playground toggles
- Checked Code Sandbox works for any docs examples
Code quality checklist
- Added or updated jest and cypress tests

JasonStoltz · 2024-05-06T20:01:32Z

We should update https://github.com/elastic/eui/blob/main/wiki/contributing-to-eui/running-eui-locally.md to indicate at the top that everything is relative to the package/eui directory. Otherwise, folks will likely try to run yarn start from the project root.

JasonStoltz · 2024-05-06T20:04:23Z

package.json

+  "version": "1.0.0",
+  "description": "Elastic Design System",
+  "scripts": {
+    "pre-push": "yarn --cwd packages/eui pre-push"


Can you explain why pre-push is kept at the project root?

The way pre-push npm dependencies work is they replace the original (usually empty) .git/hooks/pre-push file with their script calling a Node.js script handling executing scripts defined in package.json and similar. There's one gotcha with this behavior - if a monorepo has multiple of these pre-push "managers" configured to initialize on yarn install, they'll override each other. That's why I opted for a single, root-defined pre-push library that will call child packages (like packages/eui or packages/website) instead.

JasonStoltz · 2024-05-06T20:09:55Z

package.json

@@ -0,0 +1,19 @@
+{
+  "name": "@elastic/eui-monorepo",


Can you also explain why yarn.lock needs to be at the root level?

It would seem to me that it must be at the same level as the package.json file. Otherwise, I don't understand how packages are properly kept isolated from one another. Theoretically, different packages could have different versions of different dependencies, and wouldn't a single yarn.lock file mess that up?

From what I read all dependencies are hoisted to the root level and yarn workspaces requires a single yarn.lock at the root, does that sound correct?

That's correct! There can be only one yarn.lock per project, and our project is now defined by the root-level private package. Yarn uses hoisting to store the dependencies at the root level and reuse them for all packages within the whole monorepo (assuming the semver of these packages matches).

There's a way to opt-out from this hoisting behavior in case of dependencies that specifically require being stored at the correct node_modules directory, e.g., eslint-plugin-local (it cannot resolve the correct .eslintplugin.js when stored in the root node_modules dir) or stylelint (it cannot resolve customSyntax dependencies).

JasonStoltz · 2024-05-06T20:14:42Z

yarn.lock

-  resolved "https://registry.yarnpkg.com/react-focus-lock/-/react-focus-lock-2.9.5.tgz#8a82f4f0128cccc27b9e77a4472e8a22f1b52189"
-  integrity sha512-h6vrdgUbsH2HeD5I7I3Cx1PPrmwGuKYICS+kB9m+32X/9xHRrAbxgvaBpG7BFBN9h3tO+C3qX1QAVESmi4CiIA==
+react-focus-lock@^2.9.4, react-focus-lock@^2.9.5:
+  version "2.12.1"


Did you intend to update these version in yarn.lock?

This was updated when I corrected the resolutions field by moving it back to the root-level package.json. I realized it doesn't work when defined at package level (e.g., packages/eui).

The commit message of 14af611 explains that :)

OK. I didn't really gather that from the commit message. So it is OK that our yarn.lock file is updating from the current version of 2.9.5 to 2.12.1?

You were right, this minor version bump caused that cypress test to fail. I fixed it in 03f8dc9

ℹ️ 🔗 This Snyk upgrade PR is related to this issue

JasonStoltz · 2024-05-06T20:20:16Z

Have you tested the publishing process yet?

JasonStoltz

Looks like everything is working well so this LGTM. Any idea why we have failing tests? Not sure if it could be related to the react-focus-on update?

1) Cell focus utils
--
  | correctly toggles the header cell actions on focus when tabbing back into the datagrid:
  |  
  | Timed out retrying after 4000ms
  | + expected - actual
  |  
  | -'dataGridKeyboardShortcutsButton'
  | +'dataGridHeaderCellActionButton-column'
  |  
  | at assertFocusedHeaderActions (webpack://@elastic/eui/./src/components/datagrid/body/cell/focus_utils.spec.tsx:195:19)
  | at Context.<anonymous> (webpack://@elastic/eui/./src/components/datagrid/body/cell/focus_utils.spec.tsx:222:4)

JasonStoltz

Actually going to remove my approvals until we get the failing specs figured out

tkajtoch · 2024-05-08T13:38:27Z

@JasonStoltz I updated running-eui-locally.md to cover the monorepo changes

…when testing and building EUI

…irectory for correct dependency version resolution

…curately on package level

…dcoded `../../.eslintplugin.js` module resolution

…tom syntax rules can be resolved properly

…n build` and `yarn start`

mgadewoll · 2024-05-10T12:21:32Z

package.json

-    "pre-push": "yarn --cwd packages/eui pre-push"
+    "pre-push": "yarn --cwd packages/eui pre-push",
+    "preinstall": "echo \"\\x1b[K\\x1b[37;41mWarning: EUI has recently migrated to a monorepo structure. Please run EUI scripts like \\x1b[1;4myarn start\\x1b[0m\\x1b[37;41m or \\x1b[1;4myarn build\\x1b[0m\\x1b[37;41m from the \\x1b[1;4mpackages/eui\\x1b[0m\\x1b[37;41m directory instead!\n\nIf this is the first time you're running EUI after the monorepo migration, please run this first from the root repository's directory to clean up your local environment:\n  \\x1b[1;4mrm -rf node_modules .cache-loader dist es lib optimize test-env types .eslintcache eui.d.ts && yarn\\x1b[0m\\x1b[37;41m\n\nInstall process will continue in 10 seconds...\\x1b[0m\"; sleep 10",
+    "start": "echo '\\x1b[K\\x1b[37;41mPlease run this script from the \\x1b[1;4mpackages/eui\\x1b[0m\\x1b[37;41m directory instead\\x1b[0m'; exit 1",


Really nice thinking to add a warning here 🎉 Worked like a charm and caught me doing silly things! 🐟

mgadewoll · 2024-05-10T12:33:06Z

Awesome work on the monorepo setup! 🎉
I ran the PR locally and everything seems to work as expected:

builds run and built outputs can be served and run as expected
all unit and cypress tests pass
linting runs as expected and passed
the local development environments build and run as expected (Storybook and EUI docs)

JasonStoltz

LGTM

…ith the current setup

…and hoisted dependencies

kibanamachine · 2024-05-13T10:19:40Z

Preview staging links for this PR:

Docs site: https://eui.elastic.co/pr_7736/
Storybook: https://eui.elastic.co/pr_7736/storybook

elasticmachine · 2024-05-13T10:19:44Z

💚 Build Succeeded

Buildkite Build
Commit: 208e430

History

💚 Build #1935 succeeded 334f250
💔 Build #1932 failed 334ffe3
💚 Build #1926 succeeded 9e89ea2
💚 Build #1915 succeeded b817ed2
💚 Build #1913 succeeded 4ef8ba4
💔 Build #1895 failed d408a55

cc @tkajtoch

tkajtoch self-assigned this May 6, 2024

tkajtoch added the skip-changelog Use on PRs to skip changelog requirement (Don't delete - used for automation) label May 6, 2024

tkajtoch force-pushed the feat/hello-monorepo branch from d4f12cc to b90ae15 Compare May 6, 2024 15:35

tkajtoch marked this pull request as ready for review May 6, 2024 18:03

tkajtoch requested a review from a team as a code owner May 6, 2024 18:03

JasonStoltz reviewed May 6, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

JasonStoltz approved these changes May 6, 2024

View reviewed changes

JasonStoltz requested changes May 6, 2024

View reviewed changes

tkajtoch force-pushed the feat/hello-monorepo branch from d408a55 to c50ca1d Compare May 8, 2024 13:26

tkajtoch added 11 commits May 8, 2024 15:46

build: reorganize .gitignore files

bd249bd

chore: move @elastic/eui files to packages/eui

1622b59

chore: setup pre-push in root-level private package

0be81bb

build: update buildkite pipelines to use correct working directories …

a6ea926

…when testing and building EUI

chore: add monorepo note to README

733ec74

build: enable yarn workspaces and move yarn.lock back to the root d…

c424f90

…irectory for correct dependency version resolution

feat: update enzyme-to-json serializer path

ed8e856

feat: add root node_modules/@types to tsconfig's typeRoots

44d5634

fix: apply resolutions field on the workspace level instead of inac…

8d0b254

…curately on package level

fix: add eslint-plugin-local to workspaces.nohoist due to its har…

dacc19e

…dcoded `../../.eslintplugin.js` module resolution

fix: add stylelint to nohoist so that postcss-styled-syntax cus…

a7c5def

…tom syntax rules can be resolved properly

tkajtoch added 5 commits May 8, 2024 15:46

docs: update docs with new paths

094d31d

build: update deploy pipeline scripts to use correct paths

5dd4aac

feat: add monorepo warning messages when running yarn install, `yar…

90024f5

…n build` and `yarn start`

docs: add monorepo note to running-eui-locally.md

71974b3

fix: more strict react-focus-lock version resolution

b817ed2

tkajtoch force-pushed the feat/hello-monorepo branch from 4ef8ba4 to b817ed2 Compare May 8, 2024 16:23

Merge branch 'main' into feat/hello-monorepo

9e89ea2

tkajtoch requested a review from JasonStoltz May 10, 2024 10:59

mgadewoll reviewed May 10, 2024

View reviewed changes

mgadewoll approved these changes May 10, 2024

View reviewed changes

tkajtoch added 2 commits May 10, 2024 14:53

Merge remote-tracking branch 'upstream/main' into feat/hello-monorepo

f185622

fix: improve preinstall cleanup command

7b54b74

JasonStoltz approved these changes May 10, 2024

View reviewed changes

tkajtoch added 3 commits May 10, 2024 16:34

fix: revert sub-dependency updates introduced in another PR to work w…

334ffe3

…ith the current setup

fix: update @storybook/test alias resolution to work with monorepo …

334f250

…and hoisted dependencies

chore: add jsdoc explaining the need for getAbsoluteDependencyPath

208e430

tkajtoch merged commit 5c40315 into elastic:main May 13, 2024

JasonStoltz mentioned this pull request May 21, 2024

[Meta] Monorepo and package-based publishing #5540

Closed

mgadewoll mentioned this pull request May 27, 2024

[EuiIcon] Add move glyph #7789

Merged

11 tasks

Conversation

tkajtoch commented May 6, 2024 • edited by mgadewoll Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

QA

General checklist

Uh oh!

JasonStoltz commented May 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkajtoch May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonStoltz commented May 6, 2024

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

JasonStoltz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JasonStoltz left a comment

Choose a reason for hiding this comment

Uh oh!

tkajtoch commented May 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgadewoll commented May 10, 2024

Uh oh!

JasonStoltz left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented May 13, 2024

Uh oh!

elasticmachine commented May 13, 2024

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tkajtoch commented May 6, 2024 •

edited by mgadewoll

Loading

tkajtoch May 6, 2024 •

edited

Loading

JasonStoltz left a comment •

edited

Loading