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

feat: add use-deprecated-from-deprecated #204

Merged
merged 2 commits into from
Aug 16, 2024

Conversation

joshblack
Copy link
Member

Closes https://github.com/github/primer/issues/3680

Add a rule that will help us migrate code from @primer/react to @primer/react/deprecated as the middle step in deprecating some of these components. This is not a rule that will be enabled by default. Instead, it will help us migrate code in this middle step of migration.

@joshblack joshblack requested a review from a team as a code owner July 30, 2024 18:09
@joshblack joshblack requested a review from camertron July 30, 2024 18:09
Copy link

changeset-bot bot commented Jul 30, 2024

🦋 Changeset detected

Latest commit: 30a755c

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

This PR includes changesets to release 1 package
Name Type
eslint-plugin-primer-react Minor

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

// Mixed deprecated and non-deprecated imports
{
code: `import {Button, Tooltip} from '@primer/react'`,
output: `import {Button, } from '@primer/react'
Copy link
Member

Choose a reason for hiding this comment

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

Is this , after the Button expected? Seems like tests are succeeding so maybe it is expected. If so, are we relying on prettier to remove the comma after the fix?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, let me know if you have a good way to remove it 😅 I couldn't really figure it out. Definitely relying on prettier for this

Copy link
Member

@broccolinisoup broccolinisoup left a comment

Choose a reason for hiding this comment

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

Are we thinking to use the canary version of the rule to fix the imports since we are not looking into enabling it in the downstread repo? Or do you have a different path in mind?

Thanks for raising this PR, btw. Looking great!

@joshblack
Copy link
Member Author

Are we thinking to use the canary version of the rule to fix the imports since we are not looking into enabling it in the downstread repo?

@broccolinisoup I figured that we could ship the rule but not add it to the recommended config so that we could turn it on in dotcom in our PR and autofix and then turn it off

Curious what you think 👀

@broccolinisoup
Copy link
Member

broccolinisoup commented Aug 16, 2024

I figured that we could ship the rule but not add it to the recommended config so that we could turn it on in dotcom in our PR and autofix and then turn it off

Not adding into the recommended config is a great idea! 🚀 @joshblack

@joshblack joshblack merged commit e2cab87 into main Aug 16, 2024
8 checks passed
@joshblack joshblack deleted the feat/add-use-deprecated-from-deprecated branch August 16, 2024 15:03
@primer-css primer-css mentioned this pull request Aug 16, 2024
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