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 to peer dependencies #484

Conversation

nathanfranklin
Copy link
Member

@nathanfranklin nathanfranklin commented Oct 15, 2024

Overview

@wesleyboar , this works but i don't think we should merge in to tup-ui's main but instead move your target branch (and this) to the stand-alone repo.

This main goal of this PR is PR moves react-related libraries to peerDependencies in libs/core-components:

  • react and react-dom are included in peerDependencies to avoid bundling a second instance of React and ensures compatibility with the React version used by the consuming project.
  • react-router-dom, formik, react-table, react-dropzone, etc., are also part of the React ecosystem. These libraries are placed in peerDependencies because the consuming application is expected to provide them. This avoids potential version conflicts with the versions already used by the consuming application. (Although, for some of these, we could consider moving them depending on specific use cases 🤔).

Main issue

Moving s many libraries to be peer-dependencie is the main fix for error useLocation() in the context of a <Router> issue seen in TACC-Cloud/hazmapper#264.

But it is also tricky, as it's easy to get duplicate react-router-dom code showing up in the package due to the mono repo and the global nature of node_modules and package-lock.json. I kept hitting that, and then I thought the peer dependency wasn't the correct or complete fix. But it works with the following

rm -rf node_modules package-lock.json
npm install --include=optional --workspace=libs/core-components --force
npx nx build core-components --skip-nx-cache
npm pack --workspace=libs/core-components --pack-destination=.

and then built a beta release for 0.0.3

npm version prerelease --preid=beta --workspace=libs/core-components
npm publish --dry-run --tag beta --workspace=libs/core-components

Notes

Todo

  • fix failing ci
  • ensure TUP runs with updated dependencies; package-lock.json needs to be updated. NOTE i had to tweak the dockerfile for the cms image to be built; I haven't pushed that change as not certain if its due to be being on a mac.

Also add vite for building as we want to make stand-alone
Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not fully grasp the details of the changes, but high-level — i.e. "fix dependencies, tweak build process" — this looks like an acceptable way to solve the router bug.

I commented on one dependency that I think is duplicated. After that is explained/resolved, I don't mind merging this in.

The sooner we collapse this stack of PRs (main 🛑 #463 ⬅️ #465 ⬅️ #484), the sooner we can move this to its own repo, at which point the nx/monorepo-complexity could be stripped, which I think will make the codebase more approachable.

libs/core-components/package.json Outdated Show resolved Hide resolved
@nathanfranklin nathanfranklin marked this pull request as draft October 16, 2024 18:21
@nathanfranklin nathanfranklin marked this pull request as ready for review October 16, 2024 21:28
@nathanfranklin nathanfranklin marked this pull request as draft October 16, 2024 21:28
@nathanfranklin nathanfranklin marked this pull request as ready for review October 17, 2024 01:21
@nathanfranklin
Copy link
Member Author

@wesleyboar and @sophia-massie

good to review. note the following; i:

NOTE i had to tweak the dockerfile for the cms image to be built; I haven't pushed that change as not certain if its due to be being on a mac.

i had to do so curious if just me or something needs to be fixed there:

-FROM node:20 as node_build
+FROM --platform=linux/amd64 node:20 as node_build

@nathanfranklin nathanfranklin changed the title WIP: Move to peer dependencies Move to peer dependencies Oct 17, 2024
…at/TUP-700-core-components-node-pkg--use-pkg--hazmapper
@wesleyboar
Copy link
Member

@nathanfranklin,

I resolved the merge conflicts locally. I am testing. It's a chore you needn't do. I think I got it. I'll commit when I'm sure.

Regarding FROM node:20 as node_build causing error, I got it too; I am also on a Mac. Please commit that change.

  • We will not merge this to main.
  • Committing it gives tup-ui a record of and context for the solution.

Copy link
Member

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes

  • merged in target branch (which has latest from main)
  • resolved merge conflicts (8065676)

Testing & UI

TUP-UI

  1. Run npx nx serve tup-ui and npx nx serve tup-cms
  2. Open http://localhost:8000/.
  3. Log in to Portal.
  4. Poke around,.
  5. Verify everything looks like dev server.

UI skipped.1

Core-Components Demo

  1. Run npx nx serve core-components
  2. Open http://localhost:4400/.
  3. See Button.
  4. Verify it shows four button types that respond to state change.
Video
core-components.demo.mov

UI-Patterns (Manual Demo)

  1. Run npx nx serve tup-ui.
  2. Open http://localhost:4200/.
  3. Scroll & Test.
  4. Verify stuff looks and works like before.

UI skipped.2

Footnotes

  1. Because it would include data maybe inappropriate for public.

  2. Because video was too big and this demo is outdated anyway.

@wesleyboar wesleyboar merged commit c7a3ea0 into feat/TUP-700-core-components-node-pkg--use-pkg Oct 18, 2024
1 check passed
@wesleyboar wesleyboar deleted the feat/TUP-700-core-components-node-pkg--use-pkg--hazmapper branch October 18, 2024 17:58
wesleyboar added a commit that referenced this pull request Oct 18, 2024
* fix: revert FormikFieldWrapper→FieldWrapperFormik

* fix: support .js imports

* chore: use index.js when component is .jsx

* fix: make FormikCheck useField like other fields

* fix: FieldWrapper… TACC-Cloud/Hazmapper form crash

TACC-Cloud/hazmapper's only form crashed.
If checkbox is removed, then now it doesn't.

Expect checkbox fix in next commit.

TACC-Cloud/hazmapper#239

* fix: FieldWrapper…  TACC-Cloud/Hazmapper checkbox

TACC-Cloud/hazmapper's only checkbox field crashed. Now it doesn't.

TACC-Cloud/hazmapper#239

* fix: FormikCheck not supporting ID attr

* chore: npx nx format:write

* fix: FieldWrapper… ID fallback support incomplete

* fix: remove unused prop

* refactor: use Formik field wrappers as components

* fix: FieldWrapper… ID fallback support STILL incomplete

* docs: update author, add contributors

* Move to peer dependencies (#484)

* Move to peer dependencies

Also add vite for building as we want to make stand-alone

* Make 0.0.3 prelease

* Bump postcss and postcss-preset-env to match core styles requirements

* Fix pretty issue

* Add vite-plugin-lib-inject-css

* Make uuid only as a dependeny allow any 8 or 9 version

---------

Co-authored-by: Wesley B <[email protected]>

---------

Co-authored-by: Nathan Franklin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants