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

Remove self package reference in ui-components #1123

Merged
merged 1 commit into from
Apr 14, 2021

Conversation

james-wills-sf
Copy link
Contributor

While exploring creating a npm package fork, I ran into an issue where I couldn't change the @bloom-housing org name in package.json because there was an @bloom-housing/ui-components import from within ui-components.

This seems like a mistake, so this change switches it to a local import instead. This might be something we can lint for?

@exygy-dev exygy-dev temporarily deployed to bloom-refere-chores-rem-h9qsza April 13, 2021 23:31 Inactive
@jaredcwhite
Copy link
Collaborator

Absolutely. Not sure what we'd use to lint this but happy to get something like that set up. Was it just this once instance, or are there other cases lurking about?

@james-wills-sf
Copy link
Contributor Author

@jaredcwhite I could only find the one instance. It looks like there are @bloom-housing/ui-components in ui-components/.storybook but when I changed those to relative imports storybook failed to run. I don't really know much about how storybook gets its assets and they don't break anything so I'm fine with keeping those imports as-is.

@james-wills-sf
Copy link
Contributor Author

Looked into linting this, you could use something like import-blacklist but that would require some refactoring since you'd need linting rules specific to each package directory.

Not necessary for now, maybe something to think about if it comes up again

@james-wills-sf
Copy link
Contributor Author

@jaredcwhite looks like tests are passing. I'll leave it to you to do the merge since idk bloom's merge and deploy processes

@jaredcwhite
Copy link
Collaborator

LGTM!

@jaredcwhite jaredcwhite merged commit 79040da into master Apr 14, 2021
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.

None yet

3 participants