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

Drop deprecated formatter libraries #247

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Chevindu
Copy link
Contributor

Description

This change updates intl-messageformat dependency version so that it fixes following npm deprecation warnings logged when installing react-intl-universal in a project.

npm WARN deprecated [email protected]: We've written a new parser that's 6x faster and is backwards compatible. Please use @formatjs/icu-messageformat-parser
npm WARN deprecated @formatjs/[email protected]: the package is rather renamed to @formatjs/ecma-abstract with some changes in functionality (primarily selectUnit is removed and we don't plan to make any further changes to this package
npm WARN deprecated @formatjs/[email protected]: We have renamed the package to @formatjs/intl-numberformat

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

  • Edited Test Case: HTML Message with XSS attack
  • Added Test Case: HTML Message with variables and custom XML parser

@SamuelLeclerc33
Copy link

Is this going to get merged ? @cwtuan

@Chevindu Chevindu marked this pull request as ready for review April 5, 2024 04:36
@cwtuan
Copy link
Collaborator

cwtuan commented Apr 7, 2024

Is it possible to avoid making a breaking change?

@Chevindu
Copy link
Contributor Author

Chevindu commented Apr 7, 2024

I noticed the upstream library (formatjs) does not support the full HTML spec as rich text according to this part in their docs.

We could add support for a limited set of HTML/XML tags in react-intl-universal and allow users to override it during init(). But I did not do it due to following reasons:

  • I could not think of a sensible default that would fit all use-cases of the library.
  • Maintainers of the package may have to keep up with the future requests to add inbuilt support for specific XML tags.

Thus I added the option of xmlParser to allow users to define the parsing they want to use in their messages. This part would need a documentation update, which should probably also point to the limitations mentioned in the upstream documentation.

@cwtuan
Copy link
Collaborator

cwtuan commented Apr 8, 2024

Thank you for your reply. As you mentioned, this change will require an update to the documentation. Could you please update the README?

Signed-off-by: Chevindu Wickramathilaka <[email protected]>
@Chevindu
Copy link
Contributor Author

I'm sorry about the delay. I just updated the docs. Feel free to adjust/correct my wording.

@SamuelLeclerc33
Copy link

Bump @cwtuan

@cwtuan
Copy link
Collaborator

cwtuan commented May 12, 2024

Thank you for your contribution to this upgrade. The new version of intl-messageformat shows promise in handling rich text messages effectively. However, the potential for breaking changes could lead to disruptions in existing applications.

Internally, we're actively discussing strategies to mitigate these concerns and provide developers with the necessary tools to transition smoothly. Our suggestion is developing a tool to scan application source code and assist developers in writing correct messages. We believe such a tool could be immensely valuable in helping developers identify and address any potential issues introduced by the update.

We'd love to hear more about your thoughts on this idea and any insights you might have on how we can best implement it.

@cwtuan
Copy link
Collaborator

cwtuan commented May 13, 2024

Another approach is to implement a new function such as intl.getComponent that supports rich text, while ensuring that the intl.getHTML function remains compatible with its previous behavior.

@cwtuan cwtuan mentioned this pull request May 22, 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.

3 participants