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

BREAKING: remove DeprecatedModal component #802

Merged
merged 2 commits into from
Sep 26, 2023

Conversation

pedroalves0
Copy link
Member

Short description

The DeprecatedModal component is built on top of the @reach/dialog module, which is no longer maintained and lacks support for react v18. When installing Reactist in a project using the latest React version, similar warnings to the following are thrown:

npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: @reach/[email protected]
npm WARN Found: [email protected]
npm WARN node_modules/react
npm WARN   react@"18.2.0" from the root project
npm WARN   117 more (@contentful/rich-text-react-renderer, ...)
npm WARN 
npm WARN Could not resolve dependency:
npm WARN peer react@"^16.8.0 || 17.x" from @reach/[email protected]
npm WARN node_modules/@doist/reactist/node_modules/@reach/dialog
npm WARN   @reach/dialog@"^0.16.0" from @doist/[email protected]
npm WARN   node_modules/@doist/reactist

As the component has been marked for deprecated for a long time now, it's reasonable to assume that it can be removed (alongside @reach/dialog) without major impact to consumers.

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json and package-lock.json (npm --no-git-tag-version version <major|minor|patch>) ref
  • Reviewed and approved Chromatic visual regression tests in CI

Versioning

Major version bump to v22.0.0, as the removal of an exported component is a breaking change.

@pedroalves0 pedroalves0 requested a review from a team September 26, 2023 09:06
@pedroalves0 pedroalves0 self-assigned this Sep 26, 2023
@pedroalves0 pedroalves0 requested review from rfgamaral and removed request for a team September 26, 2023 09:06
@pedroalves0 pedroalves0 changed the title Pedroa/remove reach dialog dep BREAKING: remove DeprecatedModal component Sep 26, 2023
Copy link
Member

@rfgamaral rfgamaral left a comment

Choose a reason for hiding this comment

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

I understand that these changes are being done in the context of Todoist, and although DeprecatedModal is not being used there, it's still being heavily used on Twist. This change could prevent us from upgrading Reactist on Twist without some bigger effort migrating all modals all at once.

Not exactly sure what's the best course of action here, since we need to fix these dependencies issues. Other than that, the changes are good, and consider this approved 👍

@pedroalves0
Copy link
Member Author

I'm aware that Twist still uses it @rfgamaral , but given that development on Twist is somewhat stalled, I think it's a fair compromise that we'll move on with modernizing Reactist. Given your pushback, let's expand the discussion to the team; I'll create a thread.

@rfgamaral
Copy link
Member

Given your pushback, let's expand the discussion to the team; I'll create a thread.

It's not a pushback per se, I'm just concerned that when Twist development resumes, we'll have a harder time doing so, because we might need to upgrade Reactist to the latest version for small changes/improvements, but won't be able to without first dealing with the removal DeprecatedModal that is being heavily used by it.

I think it's a fair compromise that we'll move on with modernizing Reactist.

Maybe you're right, but I just wanted to bring that to our attention.

@pedroalves0
Copy link
Member Author

Update here, our internal discussion concluded that the effort to migrate the deprecated modals on Twist is relatively small. Thus, I'm going ahead with the merge of this PR.

@pedroalves0 pedroalves0 merged commit 3321ae7 into main Sep 26, 2023
@pedroalves0 pedroalves0 deleted the pedroa/remove-reach-dialog-dep branch September 26, 2023 14:56
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