h5web icon indicating copy to clipboard operation
h5web copied to clipboard

Switch to monorepo structure

Open axelboc opened this issue 3 years ago • 2 comments

We're starting to see the limitations of our current setup:

  • @h5web/lib and @h5web/app each contain a single JS bundle with all the files and dependencies (except React). This means that apps that use these packages cannot optimize away unused code with tree-shaking.
  • If those apps need some of the dependencies already present in the bundle (like Three, R3F, D3 or zustand), those dependencies will end up duplicated in the app's output bundle.
  • Because all the dependencies are listed in a single package.json at the root of the project, there's no way to know which one is used by which package.
  • Because we don't list dependencies in @h5web/lib, they are not installed when the package is installed, which means that typings and source maps for those dependencies don't work. Listing the dependencies in @h5web/lib is tedious because of the problem above.
  • webpack isn't the most appropriate tool for building libraries yet, as it doesn't support outputting ES modules and in a one-to-one manner. Unfortunately, we cannot replace it with Babel because it's difficult to tell which files are needed for the library and need to be transpiled, and which ones aren't. Using Rollup would be possible, but difficult because of the single package.json, as it needs to know which dependencies are needed by the lib.

For these reasons, I believe the time has come to switch to a proper monorepo structure that will make the project more scalable, optimised and easier to maintain.

A few solutions are available, like Lerna, Yarn Workspaces, etc. but my eyes are currently on Rush, which is developed by Microsoft and seems like a more robust, stable, flexible and all-in-one solution than other tools.

Here is what the structure could look like:

+ .vscode
+ .github
+ node_modules
+ packages // NPM packages, each with their own Storybook, potentially
  + app
  + lib
    - src
    - package.json
    - README.md
    - etc.
  + nexus
+ demo // CRA (note that Rush doesn't force every projects and packages to be under a single folder like Lerna (`packages`)
  + cypress
  - etc.
- .env
- .eslintrc.js
-  package.json // Rush
- tsconfig.json
- etc.

axelboc avatar Jun 17 '21 07:06 axelboc

Remaining known issues/limitations/awkwardness:

  • [x] Re-enable ESLint's sort imports rule. #805
  • [x] Review global app styles (remove unused, move, refactor, etc.) #808
  • [x] Move all feature tests in one folder to workaround DomainSlider.test.tsx case? #810
  • [x] Review structure of packages (remove h5web folder, etc.) #810 #811
  • [x] Some files are not linted (e.g. cypress/**/*.ts, eslint.config.js, apps/demo, etc.) #812
  • [x] ESLint plugin in CRA doesn't warn of errors outside of demo folder. #841
  • [x] Type-checking only occurs when building apps/packages and running tests, not when linting, so typing issues are often missed until the test and e2e CI jobs fail because of them. https://github.com/silx-kit/h5web/pull/911
  • [x] ~config-overrides.js in demo and demo-cra-lib have parsing errors, and~ .eslintrc.cjs in demo/storybook should have .js extension since package.json doesn't have "type": "module".
  • [x] https://github.com/vitejs/vite/issues/3092#issuecomment-915952727 - CSS modules compilation issues with Vite on Windows (root: '.')

Unresolved:

  • https://github.com/vitejs/vite/issues/7504#issuecomment-1210538294 - CSS modules ordering issues in output styles.css when composing from relative paths -- forces us to use global utilities
    • original issue marked as duplicate: https://github.com/vitejs/vite/issues/5185
  • CodeSandbox CI to test packages before release? 👉 beta testing workflow is satisfactory -- testing in real apps is better anyway.

axelboc avatar Sep 29 '21 06:09 axelboc

For reference, the monorepo set-up has been simplified significantly in PRs #937, #938, #939, and #942. Notable changes:

  • The demo is now based on Vite instead of CRA and no longer requires a config-override.js file.
  • Environment variables REACT_APP_DIST and STORYBOOK_DIST have been removed, as testing distributed packages this way is not sufficiently reliable and not worth the complexity it adds to the monorepo. Distributed packages can be tested by publishing beta versions.
  • For the same reason, project demo-lib-cra has been removed.
  • Four Codesandboxes are now available to test the published packages reliably inside both CRA and Vite apps before publishing official releases.
  • The process of building the packages' styles is now more robust and a lot clearer, and consumers of @h5web/app now need to import a single stylesheet instead of two.
  • Building the packages can now be done by running a single script: pnpm packages (no need to run pnpm packages:dts afterwards, as this is now done automatically).

axelboc avatar Jan 28 '22 15:01 axelboc

I think I'm ready to close this. The two remaining checkboxes are not worth waiting for or resolving: the CSS Modules issue seems unsolvable, and the CodeSandbox CI is kind of superseded by our awesome beta release workflow, which allows us to test releases in real projects.

axelboc avatar Mar 30 '23 14:03 axelboc