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(react-intl): Bump react-intl package [BREAKING CHANGE] #3466

Merged
merged 27 commits into from
Feb 12, 2024

Conversation

soygitana
Copy link
Contributor

@soygitana soygitana commented Nov 28, 2023

BREAKING CHANGE
IMPORTANT:

  • Minimal set of changes to solve the problem.

Changes:

  1. react-intl lib has been upgrade to v6.4.2

  2. I have to change syntax of some messages and update related snapshots. Single quote which were use to wrap {placeholder} have been replaced by double quote. This is related to Message Format syntax changes in later versions of react-intl. Escape character has been changed to apostrophe ('). (a9984c3)

e.g.: now "'{foo}'" will output: "{foo}" in the formatted output instead of interpreting it as a foo argument.

❗ From now, please do not wrap {placeholder} in single quote. If you want to use quotes please use double quote (“{name}”).

  1. formatRelativeTime has been fixed (check comment ⬇️ for details)

  2. Legacy support has been completely removed.


Testing:

Make sure application and strings are not broken when language is changed.


For more information, please check:

@soygitana soygitana changed the title upgrade react-intl package chore: react-intl package Nov 28, 2023
@soygitana
Copy link
Contributor Author

soygitana commented Nov 28, 2023

01ebd9a:

react-intl has been rewritten in TS. Now, react-intl exposes IntlShape as an interface. I removed the InjectIntlProvidedProps, which is not provided by the package. I used IntlShape with Flow.

@greg-in-a-box greg-in-a-box changed the title chore: react-intl package chore(react-intl): Bump react-intl package Nov 28, 2023
@soygitana
Copy link
Contributor Author

soygitana commented Nov 29, 2023

[OUTDATED, solved in PR #3461]

@greg-in-a-box @jstoffan @box/webapp

It turned out that the TS version in BUIE ("^3.7.5") is not sufficient for react-intl version 6 (v6.4 requires TS v4, and v6.5 requires TS v5). As a result, there will be type incompatibility after upgrading react-intl (please refer to the lint output). Is there a plan to upgrade TS in BUIE? If not, what would be the best solution?

A. Creating a temporary workaround by generating d.ts for react-intl
B. Suspending the react-intl upgrade until TS is upgraded

@greg-in-a-box
Copy link
Contributor

@greg-in-a-box @jstoffan @box/webapp

It turned out that the TS version in BUIE ("^3.7.5") is not sufficient for react-intl version 6 (v6.4 requires TS v4, and v6.5 requires TS v5). As a result, there will be type incompatibility after upgrading react-intl (please refer to the lint output). Is there a plan to upgrade TS in BUIE? If not, what would be the best solution?

A. Creating a temporary workaround by generating d.ts for react-intl B. Suspending the react-intl upgrade until TS is upgraded

you can upgrade to 5.2.2 since this PR #3461, we are upgrading it because of storybook being upgrade

@soygitana
Copy link
Contributor Author

soygitana commented Dec 4, 2023

a107f49:

I have to add resolution for react-intl to package.json file ( "**/react-intl/**/@types/react": "^16.9.18"). react-intl has set @types/react to version 16 || 17 || 18 (https://github.com/formatjs/formatjs/blob/main/package.json#L61) so yarn install the latest one, however @types/react 17 i 18 are incompatible. It caused a mismatch of types in some components.

@soygitana
Copy link
Contributor Author

soygitana commented Dec 4, 2023

@greg-in-a-box @jstoffan @box/webapp
It turned out that the TS version in BUIE ("^3.7.5") is not sufficient for react-intl version 6 (v6.4 requires TS v4, and v6.5 requires TS v5). As a result, there will be type incompatibility after upgrading react-intl (please refer to the lint output). Is there a plan to upgrade TS in BUIE? If not, what would be the best solution?
A. Creating a temporary workaround by generating d.ts for react-intl B. Suspending the react-intl upgrade until TS is upgraded

you can upgrade to 5.2.2 since this PR #3461, we are upgrading it because of storybook being upgrade

@greg-in-a-box Great! I can see that #3461 has been already merged so I merged master into this branch and updated yarn.lock. TS upgrade and adding resolution for react-intl (960fc2d) solved TS issues.

@soygitana soygitana marked this pull request as ready for review December 4, 2023 13:33
@soygitana soygitana requested review from a team as code owners December 4, 2023 13:33
@soygitana
Copy link
Contributor Author

@greg-in-a-box During the upgrade, I noticed that many strings in the code have changed, but the i18n/en-US.properties file has never been updated. You can rebuild en-US.properties in a separate pull request.

@soygitana
Copy link
Contributor Author

soygitana commented Dec 4, 2023

@greg-in-a-box @box/desktop-owners I'm wondering if the script scripts/findReactIntlViolators.js and the check will be necessary when all Box repos are upgraded to version 6?

Copy link
Contributor

@jstoffan jstoffan left a comment

Choose a reason for hiding this comment

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

@soygitana, the change itself looks good, but I'm wondering if we can drop all the cruft supporting legacy versions of react-intl.

Please also update the PR title and commit messages to something other than chore and ensure it's marked as a BREAKING CHANGE per the semantic conventions.

@@ -372,6 +372,7 @@
}
},
"resolutions": {
"ip": "1.1.8"
"ip": "1.1.8",
"**/react-intl/**/@types/react": "^16.9.18"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be some version of 17 to match our other repositories?

Copy link
Contributor

Choose a reason for hiding this comment

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

@soygitana, what was the reasoning for the decision here?

package.json Outdated Show resolved Hide resolved
scripts/check_generated_files.sh Outdated Show resolved Hide resolved
@@ -1,4 +1,5 @@
// @flow
// @deprecated, use FormattedMessage from react-intl v6 instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we delete these components at this point since the upgrade itself is already a breaking change?

@@ -79,7 +79,7 @@ const ReadableTime = ({
const timeDiff = timestamp - Date.now();
if (Math.abs(timeDiff) <= relativeThreshold) {
if (intl.formatRelativeTime) {
// react-intl v3
// react-intl >= 3
output = intl.formatRelativeTime(timeDiff);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove all support for legacy versions?

@soygitana soygitana changed the title chore(react-intl): Bump react-intl package Breaking(react-intl): Bump react-intl package Jan 11, 2024
@soygitana soygitana changed the title Breaking(react-intl): Bump react-intl package feat(react-intl): Bump react-intl package [BREAKING CHANGE] Jan 11, 2024
@soygitana
Copy link
Contributor Author

soygitana commented Jan 11, 2024

@soygitana, the change itself looks good, but I'm wondering if we can drop all the cruft supporting legacy versions of react-intl.

Please also update the PR title and commit messages to something other than chore and ensure it's marked as a BREAKING CHANGE per the semantic conventions.

@jstoffan Regarding supporting legacy versions of react-intl:

  1. We cannot remove the FormattedCompMessage or Plural component yet. Even though it is marked as deprecated, it is still used across all Box React apps. We have a linter rule that catches this error and reports it to code owners, but I believe it will take some time to remove it across all repositories.

  2. General support for legacy versions (conditional code, etc.): Perhaps it would be better to handle this in a separate pull request? If not, will we need to merge and release all React repositories before merging BUIE? or if BUIE is consumed as a package in React repositories, we can merge and release it independently? Subsequently, we can upgrade the BUIE package in all repositories once they are updated.

@greg-in-a-box @jstoffan Let's determine whether this pull request is necessary. Dawid has added support for react-intl v6 (to solve issue in Notes), which includes a breaking change related to formatRelative. If we remove support for react-intl v2 in this (#3466) pull request and ensure proper support for v6, I believe this pull request can be closed, correct?

@greg-in-a-box greg-in-a-box removed the request for review from a team January 11, 2024 13:55
src/utils/relativeTime.ts Outdated Show resolved Hide resolved
@soygitana
Copy link
Contributor Author

soygitana commented Jan 31, 2024

cb22936
2305977
11c0afc

In order to address issues related to formattedRelativeTime (described here), I have added a helper function that can be reused to correctly format relative time. Subsequently, I had to fix components which use formattedRelativeTime as it was implemented incorrectly.

Please refer to the screenshot for context:

PresenceAvatarTooltipContent component

Currently on MASTER (assuming react-intl v6 and formattedRelativeTime are used):

Screenshot 2024-01-31 at 15 29 23

After fix in this PR:

Screenshot 2024-01-31 at 15 19 56

PresenceCollaborator component

Currently on MASTER (assuming react-intl v6 and formattedRelativeTime are used):

Screenshot 2024-01-31 at 15 29 30

After fix in this PR:

Screenshot 2024-01-31 at 15 36 58

jstoffan
jstoffan previously approved these changes Feb 1, 2024
Copy link
Contributor

@jstoffan jstoffan left a comment

Choose a reason for hiding this comment

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

LGTM. Once this merges, consumers will be required to upgrade to get new changes to BUIE. Are we ready to upgrade our other first-party repositories in short order?

@@ -372,6 +372,7 @@
}
},
"resolutions": {
"ip": "1.1.8"
"ip": "1.1.8",
"**/react-intl/**/@types/react": "^16.9.18"
Copy link
Contributor

Choose a reason for hiding this comment

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

@soygitana, what was the reasoning for the decision here?

src/features/presence/PresenceAvatarTooltipContent.tsx Outdated Show resolved Hide resolved
@soygitana
Copy link
Contributor Author

LGTM. Once this merges, consumers will be required to upgrade to get new changes to BUIE. Are we ready to upgrade our other first-party repositories in short order?

@jstoffan Currently, only repositories with pending merges are EUA + Workflow (open pull requests); all other repositories have been merged and are functioning correctly in production.

I had a conversation with @greg-in-a-box, and he mentioned that a stable release of BUIE was just made last week. He also indicated that there won't be a stable release of BUIE anytime soon (please correct me if I'm wrong, @greg-in-a-box).

From our understanding, only annotations and preview require a stable version of BUIE (correct?). Can we delay updating those two packages, or is it necessary to do so before deploying our internal repositories?

@soygitana
Copy link
Contributor Author

#3466 (comment):

@jstoffan
I have to add resolution for react-intl to package.json file ( "/react-intl//@types/react": "^16.9.18"). react-intl has set @types/react to version 16 || 17 || 18 (https://github.com/formatjs/formatjs/blob/main/package.json#L61) so yarn install the latest one, however @types/react 17 i 18 are incompatible. It caused a mismatch of types in some components.
BUIE has '@types/react' set specifically to '^16.9.18', and there are some errors reported if I use '^17'.

Copy link
Contributor

@jstoffan jstoffan left a comment

Choose a reason for hiding this comment

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

LGTM. Please verify that any remaining internal teams who upgrade BUIE regularly are ready to upgrade react-intl as well.

@greg-in-a-box greg-in-a-box removed the request for review from rlyonbox February 8, 2024 19:11
@mergify mergify bot merged commit 307c6a4 into box:master Feb 12, 2024
7 checks passed
rustam-e added a commit to rustam-e/box-ui-elements-dg that referenced this pull request Apr 2, 2024
Co-authored-by: Trevor <7311041+tjuanitas@users.noreply.github.com>

fix(docgen): update view const name

fix(dogen): update to use new camel case for the product name

fix(docgen): rename props name

fix(docgen): address code review comments

fix(docgen): fix prop

fix(docgen): address code review comments

fix(docgen): address code review comments

fix(docgen): update snapshots

fix(docgen): lint errors

fix(docgen): lint errors

fix(docgen): disable tab on file change

fix(docgen): remove vscode specific comment

fix(docgen): convert tag properties to camel case

fix(docgen): disable docgen tab on file move by default

fix(docgen): split sidebar into components, fix response parsing

fix(docgen): tests

fix(docgen): import path

fix(docgen): casing of import file

fix(docgen): move accidentally moved css file back

fix(docgen): styles and header

fix(docgen): input styles

fix(docgen): remove search input

fix(docgen): use react intl

fix(docgen): move tags section into component, use translated strings

fix(docgen): add the translation string

fix(docgen): update tests

fix(docgen): add loading and empty state

feat(docgen): align the empty and error states with designs

fix(docgen): update snapshots

fix(docgen): convert docgen sidebar to a functional component

fix(docgen): remove comments

fix(docgen): convert to typescript

fix(docgen): spacing

fix(docgen): tests

fix(docgen): autoimport

fix(docgen): use sass variables for colors and font sizes

fix(docgen): svg props, use existing loading state

chore(docgen): add error state test

fix(docgen): ts error

fix(docgen): error message

chore(docgen): update snapshot

chore(docgen): update response fields to snake case

chore(react-beautiful-dnd): Bump react-beautiful-dnd (box#3487)

chore(react-textarea-autosize): Bump react-textarea-autosize (box#3488)

chore(npm): override draft-js (box#3486)

chore(npm): override draft-js react-tether

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

fix(content-explorer): Remove lingering contentPreviewProps (box#3489)

fix(content-explorer): Remove lingering contentPreview prop

chore: upgrade webpack and babel packages (box#3491)

* chore: upgrade webpack and babel packages

* fix: override terser-webpack-plugin versions

feat(modal): enable responsive behavior (box#3492)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

feat(usm): enable responsive behavior (box#3494)

* feat(usm): enable responsive behavior

* refactor: organize styles and specificity

fix(content-explorer): show pagination count for small devices (box#3498)

feat(content-sidebar): enable responsiveness for Preview and Sidebar (box#3497)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

chore(jest): Bump jest to ^29.7.0 (box#3493)

* chore(jest): Bump jest to 29.5.0

* chore(snapshots): updated tests

* chore(jest): feedback

* chore(jest): ci cd config

feat(content-explorer): allow not using Portal when rendering modal (box#3501)

* feat(content-explorer): allow not using Portal when rendering modal

* feat(content-explorer): update test component renderer

chore(ci): moved flow to its own job (box#3504)

chore(mocha): removed mocha (box#3506)

feat(react-intl): Bump react-intl package [BREAKING CHANGE] (box#3466)

* feat(react-intl): upgrade package

BREAKING CHANGE: upgrade the major version of the react-intl dependency

* fix: fix intl type

* fix: fix message syntax

* fix: fix comments and i18n readme file

* fix: fix react-intl version comments

* fix: use IntlShape as a type in js files

* Fix: correct type import for IntlShape

* fix: add resolution

* fix: use exact version to match other repos

* Fix: correct type import for IntlShape

* fix: remove all support for legacy versions

* fix: add relativeTime helper function

* fix: fix relative time in PresenceAvatarTooltipContent component

* fix: fix relativeTime in PresenceCollaborator component

* fix: use inclusive inequalities in relativeTime helper

* fix: remove unnecessary modulo operations

* fix: fix relative time in ReadableTime + fix snapshots

* fix: fix relative time in lastModifiedByCellRenderer

* fix: fix tests

* fix: use js

* fix: fix import

* fix: fix else statement

* fix: revert import reorder

fix(i18n): update translations (box#3508)

fix(i18n): update translations (box#3509)

feat(docgen): map tags to json paths and render a json tree, fix tests

chore(docgen): fix lint error

chore(chromatic): Enabling Chromatic (box#3503)

* chore(chromatic): Enabling Chromatic

* chore(chromatic): clean

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

fix(docgen): use translations

fix(docgen): add translation strings

fix(docgen):  tests to use translated strings

refactor(docgen):  move metadata fetching logic out of BUIE

fix(docgen): check if docgen tempalte only if feature is enabled

fix(docgen): run the is docgen check on prop change

fix(docgen): make props optional, remove duplicate props

fix(docgen): address some of the code review comments

fix(docgen): address some of the code review comments

fix(docgen): add tests to sidebar

fix(docgen): add tests to sidebar nav

fix(docgen): add tests to sidebar utils

fix(docgen): update copy

fix(docgen): update translations

fix(docgen): update json tag conversion logic

fix(docgen): remove test data

fix(docgen): remove test data

fix(docgen): update tests with fixed nesting

fix(docgen): move docgen props into docGenSidebarProps property

fix(docgen): ts warnings

chore(codeowners): add design system as owner for styles (box#3511)

fix(docgen): address code review comments

fix(docgen): update snapshot

fix(docgen): address more code review comments

fix(docgen): address code review comments

fix(docgen): address code review comments

fix(docgen): use suit convention

fix(docgen): use different icon

fix(docgen): use suit convention

fix(docgen): remove unnecessary optional checks, add default value

fix(docgen): update css class name

fix(docgen): display error state

fix(docgen): test

fix(docgen): fix styles

fix(docgen): update metadata

fix(docgen): address more code review comments

fix(docgen): move mocks into a mock file

fix(docgen): rename the prop in sidebar nav
rustam-e added a commit to rustam-e/box-ui-elements-dg that referenced this pull request Apr 2, 2024
fix(docgen): styles and header

fix(docgen): input styles

fix(docgen): remove search input

fix(docgen): use react intl

fix(docgen): move tags section into component, use translated strings

fix(docgen): add the translation string

fix(docgen): update tests

fix(docgen): add loading and empty state

feat(docgen): align the empty and error states with designs

fix(docgen): update snapshots

fix(docgen): convert docgen sidebar to a functional component

fix(docgen): remove comments

fix(docgen): convert to typescript

fix(docgen): spacing

fix(docgen): tests

fix(docgen): autoimport

fix(docgen): use sass variables for colors and font sizes

fix(docgen): svg props, use existing loading state

chore(docgen): add error state test

fix(docgen): ts error

fix(docgen): error message

chore(docgen): update snapshot

chore(docgen): update response fields to snake case

chore(react-beautiful-dnd): Bump react-beautiful-dnd (box#3487)

chore(react-textarea-autosize): Bump react-textarea-autosize (box#3488)

chore(npm): override draft-js (box#3486)

chore(npm): override draft-js react-tether

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

fix(content-explorer): Remove lingering contentPreviewProps (box#3489)

fix(content-explorer): Remove lingering contentPreview prop

chore: upgrade webpack and babel packages (box#3491)

* chore: upgrade webpack and babel packages

* fix: override terser-webpack-plugin versions

feat(modal): enable responsive behavior (box#3492)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

feat(usm): enable responsive behavior (box#3494)

* feat(usm): enable responsive behavior

* refactor: organize styles and specificity

fix(content-explorer): show pagination count for small devices (box#3498)

feat(content-sidebar): enable responsiveness for Preview and Sidebar (box#3497)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

chore(jest): Bump jest to ^29.7.0 (box#3493)

* chore(jest): Bump jest to 29.5.0

* chore(snapshots): updated tests

* chore(jest): feedback

* chore(jest): ci cd config

feat(content-explorer): allow not using Portal when rendering modal (box#3501)

* feat(content-explorer): allow not using Portal when rendering modal

* feat(content-explorer): update test component renderer

chore(ci): moved flow to its own job (box#3504)

chore(mocha): removed mocha (box#3506)

feat(react-intl): Bump react-intl package [BREAKING CHANGE] (box#3466)

* feat(react-intl): upgrade package

BREAKING CHANGE: upgrade the major version of the react-intl dependency

* fix: fix intl type

* fix: fix message syntax

* fix: fix comments and i18n readme file

* fix: fix react-intl version comments

* fix: use IntlShape as a type in js files

* Fix: correct type import for IntlShape

* fix: add resolution

* fix: use exact version to match other repos

* Fix: correct type import for IntlShape

* fix: remove all support for legacy versions

* fix: add relativeTime helper function

* fix: fix relative time in PresenceAvatarTooltipContent component

* fix: fix relativeTime in PresenceCollaborator component

* fix: use inclusive inequalities in relativeTime helper

* fix: remove unnecessary modulo operations

* fix: fix relative time in ReadableTime + fix snapshots

* fix: fix relative time in lastModifiedByCellRenderer

* fix: fix tests

* fix: use js

* fix: fix import

* fix: fix else statement

* fix: revert import reorder

fix(i18n): update translations (box#3508)

fix(i18n): update translations (box#3509)

feat(docgen): map tags to json paths and render a json tree, fix tests

chore(docgen): fix lint error

chore(chromatic): Enabling Chromatic (box#3503)

* chore(chromatic): Enabling Chromatic

* chore(chromatic): clean

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

fix(docgen): use translations

fix(docgen): add translation strings

fix(docgen):  tests to use translated strings

refactor(docgen):  move metadata fetching logic out of BUIE

fix(docgen): check if docgen tempalte only if feature is enabled

fix(docgen): run the is docgen check on prop change

fix(docgen): make props optional, remove duplicate props

fix(docgen): address some of the code review comments

fix(docgen): address some of the code review comments

fix(docgen): add tests to sidebar

fix(docgen): add tests to sidebar nav

fix(docgen): add tests to sidebar utils

fix(docgen): update copy

fix(docgen): update translations

fix(docgen): update json tag conversion logic

fix(docgen): remove test data

fix(docgen): remove test data

fix(docgen): update tests with fixed nesting

fix(docgen): move docgen props into docGenSidebarProps property

fix(docgen): ts warnings

chore(codeowners): add design system as owner for styles (box#3511)

fix(docgen): address code review comments

fix(docgen): update snapshot

fix(docgen): address more code review comments

fix(docgen): address code review comments

fix(docgen): address code review comments

fix(docgen): use suit convention

fix(docgen): use different icon

fix(docgen): use suit convention

fix(docgen): remove unnecessary optional checks, add default value

fix(docgen): update css class name

fix(docgen): display error state

fix(docgen): test

fix(docgen): fix styles

fix(docgen): update metadata

fix(docgen): address more code review comments

fix(docgen): move mocks into a mock file

fix(docgen): rename the prop in sidebar nav
rustam-e added a commit to rustam-e/box-ui-elements-dg that referenced this pull request Apr 2, 2024
* feat(react-intl): upgrade package

BREAKING CHANGE: upgrade the major version of the react-intl dependency

* fix: fix intl type

* fix: fix message syntax

* fix: fix comments and i18n readme file

* fix: fix react-intl version comments

* fix: use IntlShape as a type in js files

* Fix: correct type import for IntlShape

* fix: add resolution

* fix: use exact version to match other repos

* Fix: correct type import for IntlShape

* fix: remove all support for legacy versions

* fix: add relativeTime helper function

* fix: fix relative time in PresenceAvatarTooltipContent component

* fix: fix relativeTime in PresenceCollaborator component

* fix: use inclusive inequalities in relativeTime helper

* fix: remove unnecessary modulo operations

* fix: fix relative time in ReadableTime + fix snapshots

* fix: fix relative time in lastModifiedByCellRenderer

* fix: fix tests

* fix: use js

* fix: fix import

* fix: fix else statement

* fix: revert import reorder
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants