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

Migrate off of compound components to standalone components #2464

Open
3 tasks
viktorrusakov opened this issue Jul 23, 2023 · 3 comments
Open
3 tasks

Migrate off of compound components to standalone components #2464

viktorrusakov opened this issue Jul 23, 2023 · 3 comments
Assignees
Labels
code health Proactive technical investment via refactorings, removals, etc. enhancement Relates to new features or improvements to existing features

Comments

@viktorrusakov
Copy link
Contributor

viktorrusakov commented Jul 23, 2023

As was discovered by #2425, compound components that are extensively used by Paragon do not support tree-shaking (e.g., importing Form component would also include From.Group component into the resulting bundle even if From.Group is not explicitly used in the code, same goes for all Form.<subcomponentName> components). This negatively impacts bundles sizes of consuming applications.

It appears that there is no way to support tree-shaking for compound components (that's because all of the Form.<subcomponentName> components are part of the Form object so you have to bring them all into the bundle if you import Form), that's why popular component libraries (e.g. Material UI, Chakra UI) do not have compound components in their code, instead they utilize standalone components.

We should remove compound components from Paragon to provide best performance possible to consumers. This will result in a breaking change, so we should also provide a CLI command to ease migration.

Tasks

@viktorrusakov viktorrusakov added enhancement Relates to new features or improvements to existing features awaiting prioritization code health Proactive technical investment via refactorings, removals, etc. labels Jul 23, 2023
@adamstankiewicz
Copy link
Member

Thanks for filing this issue, @viktorrusakov! I agree with the intent of this ticket. The alternative is to continue to export both compound components alongside their associated standalone tokens like we do today, and have the choice of which to use be up to the consumer. But, I feel this is less than ideal because it leaves a path for consumers to use known less performant/optimized code, so I agree we should plan on removing the compound components.

Perhaps we go through the official DEPR process?:

  • File a DEPR issue (example)
  • Post about the DEPR issue in discuss.edx.org (example).

I'd be happy to take this part on.

If we don't provide an automated CLI tool to do this:

  • If possible, release a new feat/minor version of Paragon that includes deprecation warnings when consumers use a compound component to suggest using a standalone component instead.
  • When the stated DEPR date comes, we can then delete all the compound components, leaving only the standalone components and release a new breaking change.

If we do provide an automated CLI tool to do this:

  • Implement CLI tool to migrate from compound components to associated standalone components when run in a consumer's repo. See [BD-46] feat: add paragon-cli #2460 for a foundational start at a singular paragon CLI that supports multiple features.
  • Release breaking change with the removal of all compound components, with the release notes mentioning the CLI tool.

(i.e., no deprecation warning period necessary if we have a CLI tool, imho)

@viktorrusakov
Copy link
Contributor Author

@adamstankiewicz I think exporting both compound and standalone versions of components is not an option also because parent component would always include all of its compound components even if standalone imports are used (e.g. if someone imports just Card and CardStatus it would still bring all Card.<name> components into the bundle, and CardStatus would be included twice which is even worse than what we have now). So I think full deprecation is the only way here.

Perhaps we go through the official DEPR process?

Yeah, I also was wondering if we should. Let's do it, I'll file the DEPR issue.

I'd rather go with the CLI tool option. I would feel bad making consumers manually rename components 😅. Also this tool could be very useful in the future, seems worth creating it.

@viktorrusakov
Copy link
Contributor Author

viktorrusakov commented Jul 25, 2023

@adamstankiewicz As for validating react-bootstrap imports - we almost always import standalone components, but there are some exceptions where we import a parent component that contains compound components 😔. These components are:

  • Alert, brings Alert.Heading and Alert.Link from bootstrap which are very lightweight and usually used together with Alert, so no big deal;
  • Card, includes a lot of stuff... We do override most of its subcomponents though, e.g. we have our own Card.Header that replaces rect-bootstrap's one, BUT when we remove our own Card.Header and expose standalone CardHeader, react-bootstrap's Card.Header will still be included in the bundle. Which is bad, we will have 2 versions of header included. Considering that we don't use anything from react-bootstrap related to Card except for the parent component which just acts as a wrapper, we really should replace it with our own implementation. I'll file a separate issue for that;
  • Carousel, includes Carousel.Item and Carousel.Caption which we currently override, they are also lightweight and used together;
  • Dropdown, has 6 compound components, they are also mostly lightweight and usually used together with the parent component. Might still be worth to implement our own version.
  • Form, has 8 compound components in it and we override only 5 of them I think. Seems to be worth to implement our own version, considering that we've built a ton of custom into this component already. (and all that Form component does is just wraps its children with <form /> tag, so should be fairly easy to remove react-bootstrap's dependency here);
  • Figure, has 2 compound components, look very lightweight;
  • Nav, has 2 compound components, look very lightweight, usually used together with the parent;
  • Navbar, has 4 compound components, not sure whether they usually used, also don't look too lightweight... might be worth refactoring this one;
  • Popover, has 2 compound components, usually used together, look lightweight;
  • Toast, has 2 compound components, our docs don't give any example on their usage, so they're probably never used. I suggest to update the docs so at least people use it. Implementing our own Toast to remove compound components might not be easy / worth it.

All things considered, I think it's not too bad 🙂. We should definitely remove react-bootstrap's dependency from Card and Form components though, otherwise their compound components will start hurting us once we remove ours. (also, like 90% percent of these component are overridden by Paragon, it just doesn't make much sense to inherit from react-bootstrap anymore).

react-bootstrap reference: https://github.com/react-bootstrap/react-bootstrap/tree/v1.6.5/src (this is the current version we have in our master branch)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code health Proactive technical investment via refactorings, removals, etc. enhancement Relates to new features or improvements to existing features
Projects
Status: Backlog
Development

No branches or pull requests

2 participants