Skip to content

Conversation

rezrah
Copy link
Collaborator

@rezrah rezrah commented Oct 9, 2022

Summary

Conversion of Primer Brand to a monorepo per the ADR.

Part of https://github.com/github/primer/issues/1366

List of notable changes:

  • added the necessary configurations for an npm workspace
  • moved react, e2e, design-tokens and storybook into their own packages
  • published new package for @primer/brand-primitives
  • added custom workflow for publishing multiple packages
  • fixed previously broken start commands, now opens both docs and storybook, while running css module type generator in the background

Benchmarks

Before and after benchmarks for CI runs.

Single repo Monorepo
CI / Install, lint, format, check and tests 2m 3m
CodeQL / Analyze (javascript) 2m 1m
Integration test / Astro - React starter 4m 6m
Integration test / Create React App 4m 8m
Integration test / Next.js 6m 5m
Integration test / Remix.run 3m 3m
UI test / Accessibility 3m 5m
UI test / Primitives diff 4m 2m
Visual regression tests 4m 6m
Preview / Build & Deploy 5m 7s 9m 7s
Release canary 1m 1m

What should reviewers focus on?

  • Reviewing ergonomic differences in moving to a monorepo
  • Checking that nothing critical has been missed out

Steps to test:

  1. Checkout branch git checkout -b rezrah/npm-workspace origin/rezrah/npm-workspace
  2. npm install && cd apps/docs && npm install --legacy-peer-deps (because docs is outside workspace for now)
  3. npm run build
  4. npm run lint
  5. npm run format
  6. npm run check
  7. npm run test
  8. npm run start (should open Storybook, Docs in the browser)

Supporting resources (related issues, external links, etc):

Contributor checklist:

  • All new and existing CI checks pass
  • Tests prove that the feature works and covers both happy and unhappy paths
  • Any drop in coverage, breaking changes or regressions have been documented above
  • New visual snapshots have been generated / updated for any UI changes
  • All developer debugging and non-functional logging has been removed
  • Related issues have been referenced in the PR description

Reviewer checklist:

  • Check that pull request and proposed changes adhere to our contribution guidelines and code of conduct
  • Check that tests prove the feature works and covers both happy and unhappy paths
  • Check that there aren't other open Pull Requests for the same update/change

Screenshots:

brand-primitives and react-brand both published

@changeset-bot
Copy link

changeset-bot bot commented Oct 9, 2022

🦋 Changeset detected

Latest commit: fdf935f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 6 packages
Name Type
@primer/brand-primitives Patch
@primer/react-brand Patch
@primer/brand-e2e Patch
@primer/brand-fonts Patch
@primer/brand-config Patch
@primer/brand-storybook Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@rezrah rezrah temporarily deployed to github-pages October 9, 2022 17:04 Inactive
@rezrah rezrah temporarily deployed to github-pages October 9, 2022 22:03 Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2022

🟢 No visual differences found

Our visual comparison tests did not find any differences in the UI.

@rezrah rezrah temporarily deployed to github-pages October 9, 2022 22:28 Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Oct 10, 2022

🟢 No design token changes found

@rezrah rezrah temporarily deployed to github-pages October 10, 2022 06:51 Inactive
@rezrah rezrah temporarily deployed to github-pages October 10, 2022 08:34 Inactive
@rezrah rezrah added the brand label Oct 10, 2022
@rezrah rezrah self-assigned this Oct 10, 2022
@rezrah rezrah temporarily deployed to github-pages October 10, 2022 14:17 Inactive
@rezrah rezrah temporarily deployed to github-pages October 10, 2022 14:30 Inactive
@rezrah rezrah temporarily deployed to github-pages October 10, 2022 14:46 Inactive
@rezrah rezrah temporarily deployed to github-pages October 11, 2022 11:12 Inactive
@rezrah rezrah temporarily deployed to github-pages October 12, 2022 10:22 Inactive
@rezrah
Copy link
Collaborator Author

rezrah commented Oct 12, 2022

Thanks for trying it out @joshblack! Love the feedback.

One area to explore could be top-level configuration for things like prettier, tests, linting, TS, etc. In other words, instead of each workspace defining the config, there is a top-level config

Prettier and ESLint configs are already in the monorepo root, or are you referring to something else? Jest config however lives in React for now, because the config was mostly React-oriented. I can look at hosting out anything generic to the top-level though. Leave that with me. Does that cover it?

One task that could be helpful in the future is a watch task that lets you work with built packages across workspaces

Great point. The start command is meant to do just this! But I just noticed it wasn't actually working so the latest commit contains fixes for the watchers. Right now, I'm parallelising the storybook, gatsby and css module type generator processes. I think that should cover most scenario's for development and Storybook has it's own HMR for React components. Is there anything I've missed?

Another situation that comes up when installing create-react-app to apps is @testing-library/react which depends on React 18 but

Thanks for the Loom demo, it's certainly a confusing issue.

I've already looked at bumping the React libraries to v18. Storybook was the only blocker during that attempt, but they've very recently shipped support for it so I may just give the upgrade another go.

One the other hand, I do wonder whether create-react-app and other e2e integrations should even resolve modules at a workspace level. My thinking is to keep them isolated and free of side-effects, like docs is currently. Reason being, they are meant to represent clean installations and it's harder to make them deterministic or representative of an end-user experience otherwise. Appreciate it goes against a workspaces inherent dependency resolution, but I see it as an intentional feature, rather than a bug. For context, the current integration tests for those frameworks are ephemerally generated, which gives higher confidence that the library will work e2e.

For packages, would there ever be a use-case to use "exports" in package.json?
Would there ever be a use-case to pubish ESM bundles along with the existing CommonJS?
Would there ever be a use-case to pubish ESM bundles along with the existing CommonJS?

Absolutely. We haven't needed to solve this yet, because Primer Brand is typically used in SSR sites first, so CommonJS provided the maximum compatibility, particularly with Next.js which only recently gained support for ESM under an experimental flag. Initially I went the ESM-first route.. but the e2e compatibility just wasn't there at the time.

The current Webpack build config is flexible and we can add support for a separate ESM bundle whenever we're ready. Same applies to code splitting. I just kicked this problem down the road for now.

@rezrah rezrah temporarily deployed to github-pages October 12, 2022 12:39 Inactive
@rezrah rezrah temporarily deployed to github-pages October 12, 2022 13:21 Inactive
@rezrah rezrah temporarily deployed to github-pages October 12, 2022 13:29 Inactive
@rezrah rezrah marked this pull request as ready for review October 12, 2022 13:35
@rezrah rezrah changed the title Conversion to monorepo Convert Primer Brand to a monorepo Oct 12, 2022
@rezrah rezrah temporarily deployed to github-pages October 12, 2022 14:00 Inactive
@rezrah rezrah temporarily deployed to github-pages October 12, 2022 14:24 Inactive
@JoshBowdenConcepts
Copy link
Contributor

There seems to be a peer dependency issue when following the install steps that you provided. This could then be mitigated using the legacy dependencies flag. After that issue there was another:
Can’t resolve ‘../../../../../../packages/react/lib/css/main.css’ in ‘/Users/jbowden/Documents/brand/apps/docs/src/@primer/gatsby-theme-doctocat/components’
I believe I have run into these issue before. But I am not sure the approach to resolve them.

@rezrah
Copy link
Collaborator Author

rezrah commented Oct 12, 2022

There seems to be a peer dependency issue when following the install steps that you provided.

@JoshBowdenConcepts I've updated the description. The --legacy-peer-deps is an existing, known issue.

Can’t resolve ‘../../../../../../packages/react/lib/css/main.css’ in ‘/Users/jbowden/Documents/brand/apps/docs/src/@primer/gatsby-theme-doctocat/components’

Is this when running npm run build? I can't seem to reproduce this. the build and start commands will create the css bundle before trying to run Gatsby compiler. Maybe your webpack build failled?

@JoshBowdenConcepts
Copy link
Contributor

Is this when running npm run build? I can't seem to reproduce this. the build and start commands will create the css bundle before trying to run Gatsby compiler. Maybe your webpack build failled?

This is when running the build command. And yes it seems that webpack has failed. I am not sure why I seem to be running into these issues more often than others haha

@rezrah rezrah temporarily deployed to github-pages October 13, 2022 13:11 Inactive
@rezrah rezrah merged commit fcef134 into main Oct 13, 2022
@rezrah rezrah deleted the rezrah/npm-workspace branch October 13, 2022 15:33
@primer-css primer-css mentioned this pull request Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants