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

Fix-circular-dependencies #3955

Merged
merged 14 commits into from
Aug 10, 2023
Merged

Fix-circular-dependencies #3955

merged 14 commits into from
Aug 10, 2023

Conversation

gyfchong
Copy link
Contributor

@gyfchong gyfchong commented Aug 3, 2023

Why

We shouldn't have circular deps

What

Switched importing of components and types to rely on the source which exports it, rather than the root level index which also exports the same things.

Note

Couldn't figure out the FilterBar circular types dep :(

@changeset-bot
Copy link

changeset-bot bot commented Aug 3, 2023

🦋 Changeset detected

Latest commit: 716f218

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

This PR includes changesets to release 1 package
Name Type
@kaizen/components 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

@gyfchong gyfchong marked this pull request as ready for review August 9, 2023 02:53
@gyfchong gyfchong requested a review from a team as a code owner August 9, 2023 02:53
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

✨ Here is your branch preview! ✨

Last updated for commit 716f218: Merge branch 'main' into fix-circular-dependencies

@gyfchong gyfchong enabled auto-merge (squash) August 9, 2023 03:52
@@ -1 +1,2 @@
export * from "./Button"
export * from "./components/GenericButton"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we're now exporting the GenericButton? Makes sense for exporting the types but I wonder if it mean peeps will start trying to consume this / or if it was possible to access this before 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't look like GenericButton was available before, but this approach aligns with the new development pattern where we export internals to allow for custom builds.

If consumers actually need a custom button, then they should be able to create one. Not that the GenericButton is particularly good lol

Copy link
Contributor

Choose a reason for hiding this comment

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

True, wasn't sure if we wanted to offer it but certainly not apposed 👍

@gyfchong gyfchong merged commit 327fbe9 into main Aug 10, 2023
@gyfchong gyfchong deleted the fix-circular-dependencies branch August 10, 2023 22:06
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