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: enable html preprocessing #1529

Merged
merged 2 commits into from Jul 6, 2022

Conversation

nilsmehlhorn
Copy link
Contributor

Checklist

  • only relevant code is changed (make a diff before you submit the PR)
  • run tests npm run test
  • tests are included
  • commit message and code follows the Developer's Certification of Origin

Checklist (for documentation change)

  • only relevant documentation part is changed (make a diff before you submit the PR)
  • motivation/reason is provided
  • commit message and code follows the Developer's Certification of Origin

Closes #1435

@coveralls
Copy link

coveralls commented Jul 3, 2022

Coverage Status

Coverage increased (+0.02%) to 95.862% when pulling 5f935c3 on nilsmehlhorn:feat/preprocess-html into 3f26db0 on i18next:master.

@adrai
Copy link
Member

adrai commented Jul 3, 2022

probably major version, right?

@adrai adrai requested review from jamuhl and pedrodurek July 3, 2022 16:13
@nilsmehlhorn
Copy link
Contributor Author

probably major version, right?

Breaking change in the component interface so I'd say yes

@jamuhl
Copy link
Member

jamuhl commented Jul 3, 2022

Is there a specific reason to remove the existing functionality to unescape and making this a breaking version.

My suggestion would be:

  1. include the html-escaper function (copy&paste - not sure why this was included in first place as the function is too small to justify adding it as a dependency) => https://github.com/WebReflection/html-escaper/blob/master/index.js
  2. let devs override that unescape function

Also I would keep the unescape - naming it preprocessor makes it sound like it gets applied to all content - while it only gets applied on text nodes.

@adrai
Copy link
Member

adrai commented Jul 3, 2022

fyi: #1426

@nilsmehlhorn
Copy link
Contributor Author

include the html-escaper function (copy&paste - not sure why this was included in first place as the function is too small to justify adding it as a dependency) => https://github.com/WebReflection/html-escaper/blob/master/index.js

That entity subset is actually too small for me and others (see #1435). There's a bunch more entities, e.g. visible in alt dependency: https://github.com/mdevils/html-entities/blob/master/src/named-references.ts

let devs override that unescape function

So, like this?

type TransProps =  {
  ...
  shouldUnescape?: boolean | (text: string) => string;
}

Also I would keep the unescape - naming it preprocessor makes it sound like it gets applied to all content - while it only gets applied on text nodes.

Naming is actually shouldUnescape, which doesn't necessarily work well for function but idk. Could also go with textPreprocessor.

@jamuhl
Copy link
Member

jamuhl commented Jul 4, 2022

@nilsmehlhorn adding the unescape "default" function as option here: https://github.com/i18next/react-i18next/blob/master/src/context.js#L3 -> devs can override it by passing it in i18next.init as options.react

here https://github.com/i18next/react-i18next/blob/master/src/Trans.js#L252 instead of using unescape from the import we use it from the i18nOptions

@nilsmehlhorn
Copy link
Contributor Author

I've implemented a default unescape analogue to html-escaper and enabled the override via options. Not quite sure about the typing though, because ReactOptions seems to come from the i18n package itself which doesn't really add up for me - do we need a change in that package to update the options type?

@adrai
Copy link
Member

adrai commented Jul 5, 2022

I've implemented a default unescape analogue to html-escaper and enabled the override via options. Not quite sure about the typing though, because ReactOptions seems to come from the i18n package itself which doesn't really add up for me - do we need a change in that package to update the options type?

Yes, currently the types needs to be updated here for react-i18next: https://github.com/i18next/i18next/blob/master/index.d.ts#L186 correct me @pedrodurek if I'm wrong

@adrai adrai merged commit c840421 into i18next:master Jul 6, 2022
@adrai
Copy link
Member

adrai commented Jul 6, 2022

released with react-i18next v11.18.0 and i18next v21.8.13

@adrai
Copy link
Member

adrai commented Jul 6, 2022

@nilsmehlhorn thank you for your contribution

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.

Unescape soft-hypen
4 participants