refactor: avoid wrapping components with injectIntl #1413

emmenko · 2019-08-12T13:33:20Z

TL;DR: I'm not aware of a way to "forward" the types to the wrapped components when using a HOC. To keep things simple, and avoid using the HOC in the first place, we can simply use the context consumer directly.

I also refactored a bit the tests to use console.error = jest.fn(), which is the easiest way to mock the log.

emmenko · 2019-08-12T13:34:44Z

package.json

@@ -34,15 +34,15 @@
  "module": "./lib/index.js",
  "types": "./dist/index.d.ts",
  "dependencies": {
-    "@formatjs/intl-relativetimeformat": "^2.6.2",
+    "@formatjs/intl-relativetimeformat": "^2.6.3",


I included those deps updates as I saw they were freshly published. Let me know if it's ok, otherwise I can revert.

emmenko · 2019-08-12T13:35:12Z

src/components/html-message.tsx

-    return <Component dangerouslySetInnerHTML={html} />;
+    return (
+      <Context.Consumer>
+        {intl => {


This is basically everything that we need from the injectIntl

emmenko · 2019-08-12T13:35:32Z

src/components/message.tsx

 import {formatMessage as baseFormatMessage} from '../format';
 import {
  invariantIntlContext,
  DEFAULT_INTL_CONFIG,
  createFormatters,
 } from '../utils';
-import {PrimitiveType, FormatXMLElementFn} from 'intl-messageformat';
+const shallowEquals = require('shallow-equal/objects');


Just reordering a bit the imports

emmenko · 2019-08-12T13:35:48Z

src/components/message.tsx

-    } = this.props;
+    return (
+      <Context.Consumer>
+        {intl => {


Same thing here

emmenko · 2019-08-12T13:36:17Z

src/components/relative.tsx

-      }
+    return (
+      <Context.Consumer>
+        {intl => {


Also for this one, no need to have a HOC here

emmenko · 2019-08-12T13:37:13Z

test/unit/components/date.tsx

+    expect(console.error).toHaveBeenCalledTimes(1);
+    expect(console.error).toHaveBeenCalledWith(
+      expect.stringContaining(
+        'Error formatting date.\nRangeError: Value invalid out of range for dateformat options property year'


I added more expectations to check for the error message, as it gives a better understanding of the expected behavior.

emmenko · 2019-08-12T13:40:50Z

test/unit/components/provider.tsx

-import withIntl, {Provider} from '../../../src/components/injectIntl';
+import withIntl from '../../../src/components/injectIntl';
+
+jest.mock('intl-locales-supported');


This is necessary otherwise the test fails because the intl polyfill (for the tests) returns false for some of the intl contructors as they don't support languages other than en.

test/unit/components/provider.tsx

longlho · 2019-08-12T13:49:22Z

overall LGTM! Ready to merge once build if fixed. Thanks a lot for your contributions!

emmenko · 2019-08-12T14:05:15Z

Awesome, glad to help!

emmenko · 2019-08-12T14:12:59Z

Hmm wait, if I run npm test locally I get this:

❯ npm test                                     

> react-intl@3.1.6 test /Users/emmenko/dev/src/react-intl
> cross-env NODE_ICU_DATA=./node_modules/full-icu jest --coverage --verbose

node: could not initialize ICU (check NODE_ICU_DATA or --icu-data-dir parameters)
npm ERR! Test failed.  See above for more details.

I've been using npm test:watch to work on the PR...did I miss some step? 🤔

emmenko · 2019-08-12T14:14:15Z

I guess that might explain the problem I had with #1413 (comment)

emmenko · 2019-08-12T14:16:00Z

Ah ok, I had to npm rebuild as I switched node versions after installing the deps the first time 😅

emmenko · 2019-08-12T14:19:21Z

Alright, I force pushed and removed the jest.mock('intl-locales-supported'); workaround.

emmenko · 2019-08-12T14:20:02Z

package.json

@@ -112,7 +112,7 @@
    "release": "standard-version",
    "test:all": "npm run lint && npm run format && npm run test",
    "test:perf": "cross-env NODE_ENV=production babel-node test/perf",
-    "test:watch": "jest --watch",
+    "test:watch": "cross-env NODE_ICU_DATA=./node_modules/full-icu jest --watch",


This is what tricked me. Now the polyfilled data is loaded also when running jest in watch mode.

~~then u prob don't need to mock intl-locales-supported right?~~ nvm u rm'ed it. Thanks!

longlho · 2019-08-12T14:36:04Z

Ah Node 12 fails bc it comes w/ native Intl.RelativeTimeFormat so u might just to adjust the err message expectation

marcesengel · 2019-08-12T14:36:54Z

The reason why injectIntl was still being used is because it allows for more flexibility in the SCU.
Just wanted to throw this out there before the merge.

longlho · 2019-08-12T14:40:09Z

@DragonRaider5 can u provide more details? I'd love to understand that use case more.

emmenko · 2019-08-12T15:07:00Z

The reason why injectIntl was still being used is because it allows for more flexibility in the SCU.

The only place where we currently use sCU is the message component. I don't see a problem with not using injectIntl to be honest, because if intl changes, React will update the context, causing a new re-render anyway.

Unless I'm missing some other point...

* refactor: avoid wrapping components with injectIntl * test: adjust message matchers for node@12 native APIs