[v15] refactor(app-shell): use react-apollo v3 #801

emmenko · 2019-06-28T20:09:39Z

Use react-apollo v3, migrate internal queries to useQuery, fix tests.

package.json

emmenko · 2019-06-28T20:12:12Z

packages/application-shell/src/components/navbar/navbar.js

-    queryName: 'applicationsMenuQuery',
-    skipRemoteQuery: ownProps => !ownProps.environment.servedByProxy,
-    options: ownProps => ({
+export const NavBar = props => {


We're using several hooks here and the code is much more readable now!

I agree and appriciate this. However, I would also argue that we should split updating libs and refactoring code. So we only change as little as possible in dep updates and keep such changes for other PRs. I am just arguing for this in the future as we had quite a few regressions in the last weeks.

Yeah sure, I agree 👌

Just for the record, those refactorings were part of the experimental branch of using the Apollo hooks 2 months ago or something.

emmenko · 2019-06-28T20:13:01Z

packages/application-shell/src/components/navbar/navbar.js

+};
+
+// Use `withRouter` to "connect" again, to access the `location` object
+export default withRouter(NavBar);


Just one HOC left 🙌

emmenko · 2019-06-28T20:14:24Z

packages/application-shell/src/components/user-settings-menu/user-settings-menu.js

+      // We can assume here that the navbar already fetched the data, since this
+      // component gets rendered only when the user opens the menu
+      fetchPolicy: 'cache-only',
+      onError: reportErrorToSentry,


This is also interesting and I think we should do that more often. When we don't specifically handle the apollo error, we can just bind the error handler with our sentry reporter.

emmenko · 2019-06-28T20:15:07Z

packages/application-shell/src/components/user-settings-menu/user-settings-menu.spec.js

      });
    });
  });

  describe('<UserSettingsMenuBody>', () => {
    beforeEach(() => {
+      useApplicationsMenu.mockReturnValue({


Easy peasy 🤓

emmenko · 2019-06-28T20:15:46Z

packages/application-shell/src/hooks/use-applications-menu/use-applications-menu.js

+  options: {},
+};
+
+const useApplicationsMenu = (config = {}) => {


This also got nicer and easier to follow (I hope)

emmenko · 2019-07-01T07:57:14Z

So apparently we need to update to react >=16.9 and testing-library/react@>=8 in order to fix this problem

apollographql/react-apollo#3200 (comment)

montezume · 2019-07-01T08:24:23Z

Should we coordinate on making the required test fixes to MC-FE to upgrade to latest RTL there then?

emmenko · 2019-07-01T08:28:13Z

Yeah we need to discuss how we want to proceed here. This PR is not meant to be merged just yet, I just want to experiment and see what we need to do in order to support newer versions.

montezume · 2019-07-01T08:30:26Z

Either way it would be a good idea to update MC FE to latest testing library version... 😄

emmenko · 2019-07-01T08:35:50Z

Definitely! I added a comment in the v14 issue, to keep track of it: #748 (comment)

Btw, what was the problem with updating to latest RTL?

montezume · 2019-07-01T08:55:25Z

the breaking changes are listed here

https://github.com/testing-library/dom-testing-library/releases/tag/v4.0.0

getBy:* All getBy and findBy query variants now will throw an error if more than one element is returned. If this is expected, then use getAllBy (or findAllBy) instead.

emmenko · 2019-08-07T09:34:48Z

Now that the new apollo hooks are out, I'm going to pick this up again

montezume · 2019-08-07T09:38:12Z

Maybe we should update to rc version of React 16.9? I had a lot of trouble testing async hooks with the current version...

emmenko · 2019-08-16T13:02:42Z

packages/application-shell/src/components/fetch-user/fetch-user.js

-}
+const FetchUser = props => {
+  const { loading, data, error } = useQuery(LoggedInUserQuery, {
+    onError: reportErrorToSentry,


emmenko · 2019-08-16T13:03:19Z

packages/application-shell/src/components/inject-reducers/inject-reducers.js

@@ -24,7 +24,7 @@ export class InjectReducers extends React.PureComponent {
    this.setState({ areReducersInjected: true });
  }

-  UNSAFE_componentWillUnmount() {
+  componentWillUnmount() {


componentWillUnmount is not a deprecated lifecycle method. I guess this was confused with componentWillMount

Prolly a find and replace gone rogue.

emmenko · 2019-08-16T13:06:52Z

packages/application-shell/src/components/quick-access/quick-access.js

-          this.setState({ historyEntries })
-        }
+        onHistoryEntriesChange={historyEntries => {
+          if (!this.isUnmounting) {


Without this check, React throws a

Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in

emmenko · 2019-08-16T13:09:04Z

packages/l10n/src/create-l10n-injector/create-l10n-injector.tsx

@@ -82,17 +82,6 @@ export function createL10NInjector<LoadedData extends {}>({
  propLoadingKey,
  loadLocale,
 }: InjectorOptions<LoadedData>) {
-  if (process.env.NODE_ENV === 'test') {
-    if (React.version.startsWith('16.8')) {


We can get rid of this now 🎉

emmenko · 2019-08-16T13:11:35Z

Alright, this is now ready for review and merge. I fixed all the issues within the v15 branch, so let's merge this before doing any more work on the v15 branch. /cc @tdeekens

tdeekens · 2019-08-17T15:18:26Z

packages/application-shell/src/components/fetch-project/fetch-project.js

-}
+const FetchProject = props => {
+  const { loading, data, error } = useQuery(ProjectQuery, {
+    onError: reportErrorToSentry,


We didn't do this before though, right? Does Apollo maybe have a more generic way to do this e.g. in a link? So we have a SentryErrorReportingLink which enables us having all errors also from the app to be reported? Can be a follow up I just scratched my head cause we added this here now.

tdeekens · 2019-08-17T15:19:46Z

packages/application-shell/src/components/inject-reducers/inject-reducers.js

@@ -24,7 +24,7 @@ export class InjectReducers extends React.PureComponent {
    this.setState({ areReducersInjected: true });
  }

-  UNSAFE_componentWillUnmount() {
+  componentWillUnmount() {


Prolly a find and replace gone rogue.

tdeekens · 2019-08-17T15:21:39Z

packages/application-shell/src/components/navbar/navbar.js

-    queryName: 'applicationsMenuQuery',
-    skipRemoteQuery: ownProps => !ownProps.environment.servedByProxy,
-    options: ownProps => ({
+export const NavBar = props => {


I agree and appriciate this. However, I would also argue that we should split updating libs and refactoring code. So we only change as little as possible in dep updates and keep such changes for other PRs. I am just arguing for this in the future as we had quite a few regressions in the last weeks.

packages/application-shell/src/hooks/use-applications-menu/use-applications-menu.js

tdeekens · 2019-08-17T15:28:04Z

packages/application-shell/src/hooks/use-applications-menu/use-applications-menu.js

+  // We set up the apollo query as lazy, in order to execute it conditionally
+  // since hooks cannot be defined conditionally.
+  const environment = useApplicationContext(context => context.environment);
+  const [loadMenuData, { called, data }] = useLazyQuery(FetchApplicationsMenu, {


Nice! Stumbled over this for a minute. Should we maybe alias called to hasApplicationsMenuQueryBeenCalled. I don't know, would just've helped me. I think it's something which will freak me out with Apollo and hooks is that we'll have these super gernic data and called variable names around.

packages/jest-preset-mc-app/setup-tests.js

emmenko · 2019-08-19T09:26:49Z

@tdeekens anything else left from your side?

tdeekens · 2019-08-19T09:31:03Z

Not really. It's too general to apply here.

Let's not change too much in version updates in the same PR.
- The argument "it's been done in an older PR" is fine but also odd.
We should think of a better SentryErrorLink in the future

aminbenselim · 2019-08-19T07:50:05Z

packages/application-shell/src/components/navbar/navbar.spec.js

-    },
-  },
-  onMenuItemClick: jest.fn(),
+  // areProjectExtensionsEnabled: true,


why are these lines commented?

We don't use HOCs anymore, so they are not necessary.

TL;DR: we need to rewrite the tests to RTL 😅

tdeekens · 2019-08-19T11:23:42Z

Did you require another review by @amine-benselim or not @emmenko?

emmenko · 2019-08-19T11:44:26Z

Hmm no I didn't

aminbenselim · 2019-08-19T11:47:11Z

no he didnt. I was reviewing it while he merged it. so I just continued doing so to know what changed anyway.

* chore: update to react@16.9 (#966) * chore: update to react@16.9 * fix: update lifecyle methods * chore: temporary resolution for react * chore(deps): update dependency @testing-library/react to v9.1.1 (#956) * chore(deps): update dependency @testing-library/react to v8.0.9 * chore: update testing library to 9.1.1 * fix: remove cleanup-after-each hook (automatical now) * refactor: to remove path for act * refactor(app-shell): use react-apollo v3, use more hooks * refactor(app-shell): do not render hooks conditionally * chore: update react deps consistently * refactor(authenticated): do not render hook conditionally * chore: patch jest setup to avoid logging certain warnings on CI * fix: missing prop ref * fix(navbar): missing prop * refactor(app-connectors): to useQuery for fetching image regex setting * refactor(app-shell): connectors to useQuery * test(app-shell): fix setup * chore: regenerate lockfile * chore: update react-apollo to 3.0.1 * refactor(connectors): image regex tests to RTL * fix: query mocks for project suspended * fix: pin react-test-renderer to 16.9 * fix(quick-access): update state unless component is unmounting * refactor: componentWillUnmount is not a deprecated lifecycle method * test: cleanup * refactor: renaming based on feedback

* chore: update to react@16.9 (#966) * chore: update to react@16.9 * fix: update lifecyle methods * chore: temporary resolution for react * chore(deps): update dependency @testing-library/react to v9.1.1 (#956) * chore(deps): update dependency @testing-library/react to v8.0.9 * chore: update testing library to 9.1.1 * fix: remove cleanup-after-each hook (automatical now) * refactor: to remove path for act * refactor(app-shell): use react-apollo v3, use more hooks * refactor(app-shell): do not render hooks conditionally * chore: update react deps consistently * refactor(authenticated): do not render hook conditionally * chore: patch jest setup to avoid logging certain warnings on CI * fix: missing prop ref * fix(navbar): missing prop * refactor(app-connectors): to useQuery for fetching image regex setting * refactor(app-shell): connectors to useQuery * test(app-shell): fix setup * chore: regenerate lockfile * chore: update react-apollo to 3.0.1 * refactor(connectors): image regex tests to RTL * fix: query mocks for project suspended * fix: pin react-test-renderer to 16.9 * fix(quick-access): update state unless component is unmounting * refactor: componentWillUnmount is not a deprecated lifecycle method * test: cleanup * refactor: renaming based on feedback fix(app-shell): to run useEffect only once in dev mode

emmenko changed the title ~~refactor(app-shell): use react-apollo v3, use more hooks~~ [v14] refactor(app-shell): use react-apollo v3, use more hooks Jun 28, 2019

emmenko commented Jun 28, 2019

View reviewed changes

package.json Outdated Show resolved Hide resolved

emmenko commented Jun 28, 2019

View reviewed changes

emmenko mentioned this pull request Jun 28, 2019

Upcoming breaking changes (v14) #748

Closed

20 tasks

emmenko force-pushed the nm-react-apollo-v3 branch from 1edbbf3 to 979dd7b Compare July 1, 2019 08:09

emmenko mentioned this pull request Jul 1, 2019

Stricter support for react 16.8 || 16.9 tdeekens/flopflip#733

Closed

montezume mentioned this pull request Jul 1, 2019

Static types #589

Closed

24 tasks

emmenko force-pushed the v14 branch 3 times, most recently from b620008 to d275102 Compare July 4, 2019 12:15

emmenko added the 🖐 Status: On Hold label Jul 4, 2019

emmenko force-pushed the v14 branch 4 times, most recently from 058cc31 to 1be22d9 Compare July 15, 2019 12:33

emmenko changed the base branch from v14 to master July 22, 2019 12:47

emmenko force-pushed the nm-react-apollo-v3 branch from 9221df0 to efd2a8b Compare July 22, 2019 13:05

emmenko changed the title ~~[v14] refactor(app-shell): use react-apollo v3, use more hooks~~ [on hold, waiting for react@16.9] refactor(app-shell): use react-apollo v3, use more hooks Jul 31, 2019

emmenko commented Aug 16, 2019

View reviewed changes

emmenko requested review from tdeekens and aminbenselim August 16, 2019 13:09

emmenko added 🙏 Status: Dev Review Waiting for technical reviews and removed 🖐 Status: On Hold labels Aug 16, 2019

test: cleanup

b46c597

tdeekens reviewed Aug 17, 2019

View reviewed changes

refactor: renaming based on feedback

d559124

tdeekens approved these changes Aug 19, 2019

View reviewed changes

emmenko merged commit c797d74 into release/15.0.0 Aug 19, 2019

emmenko deleted the nm-react-apollo-v3 branch August 19, 2019 10:23

aminbenselim reviewed Aug 19, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v15] refactor(app-shell): use react-apollo v3 #801

[v15] refactor(app-shell): use react-apollo v3 #801

emmenko commented Jun 28, 2019 •

edited

Loading

emmenko Jun 28, 2019

tdeekens Aug 17, 2019

emmenko Aug 17, 2019

emmenko Jun 28, 2019

emmenko Jun 28, 2019

emmenko Jun 28, 2019

emmenko Jun 28, 2019

emmenko commented Jul 1, 2019

montezume commented Jul 1, 2019

emmenko commented Jul 1, 2019

montezume commented Jul 1, 2019

emmenko commented Jul 1, 2019

montezume commented Jul 1, 2019

emmenko commented Aug 7, 2019

montezume commented Aug 7, 2019

emmenko Aug 16, 2019

emmenko Aug 16, 2019

tdeekens Aug 17, 2019

emmenko Aug 16, 2019

emmenko Aug 16, 2019

emmenko commented Aug 16, 2019

tdeekens Aug 17, 2019

tdeekens Aug 17, 2019

tdeekens Aug 17, 2019

tdeekens Aug 17, 2019

emmenko commented Aug 19, 2019

tdeekens commented Aug 19, 2019

aminbenselim Aug 19, 2019

emmenko Aug 19, 2019

tdeekens commented Aug 19, 2019

emmenko commented Aug 19, 2019

aminbenselim commented Aug 19, 2019

[v15] refactor(app-shell): use react-apollo v3 #801

[v15] refactor(app-shell): use react-apollo v3 #801

Conversation

emmenko commented Jun 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmenko commented Jul 1, 2019

montezume commented Jul 1, 2019

emmenko commented Jul 1, 2019

montezume commented Jul 1, 2019

emmenko commented Jul 1, 2019

montezume commented Jul 1, 2019

emmenko commented Aug 7, 2019

montezume commented Aug 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmenko commented Aug 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmenko commented Aug 19, 2019

tdeekens commented Aug 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdeekens commented Aug 19, 2019

emmenko commented Aug 19, 2019

aminbenselim commented Aug 19, 2019

emmenko commented Jun 28, 2019 •

edited

Loading