Mattermost-server: Migrate string-refs to functional ones: this.refs.filter

Created on 16 Sep 2020  ·  6Comments  ·  Source: mattermost/mattermost-server

Context:

String references are deprecated in React, and they will eventually disappear. Since React 16.3 there is the new api to use for reference creation createRef which improved the way to handle them greatly. Callbacks refs still have some valid usage, so we might make exceptions with those, please raise the issue if you feel you need to use them.

How to migrate

Go to ./components/searchable_channel_list.jsx and look for usage of this.refs.filter. Once you find them:

  1. Remove usage this.refs.some-reference
  2. Add a this.some-reference = React.createRef()??
  3. Change code depending on ??this.refs.some-reference?? into using ??this.some-reference.current. Be aware that this might be accessed by files other than the one that defined it. Also, you might need to refactor the code to keep behaviour the same.
  4. If there are e2e tests using that functionality run them to ensure it works as expected. Optionally consider adding any new tests that might ensure it functions as expected.
  5. Look for any other this.refs used in the file and do the same.

If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

JIRA: https://mattermost.atlassian.net/browse/MM-28784

Example of migrated code


If you're interested please comment here and come join our "Contributors" community channel on our daily build server, where you can discuss questions with community members and the Mattermost core team. For technical advice or questions, please join our "Developers" community channel.

New contributors please see our Developer's Guide.

JIRA: https://mattermost.atlassian.net/browse/MM-28784

AreTechnical Debt Easy Hacktoberfest Help Wanted PR Exists

All 6 comments

Hi @Willyfrog , I'd like to work on this issue!

Awesome! let me know if you need any clarifications :)

Thanks for assigning the issue to me!

So far, I've setup the server and webapp repositories locally. During setup I noticed that make test was failiing with the following error:

Summary of all failing tests
 FAIL  components/data_prefetch/data_prefetch.test.tsx
  ● /components/data_prefetch › Should call for posts based on prefetchQueueObj and obey concurrency

    expect(jest.fn()).toHaveBeenCalledWith(...expected)

    Expected: "mentionChannel1"
    Received
           1: "currentChannelId"
           2: "mentionChannel0"

    Number of calls: 2

      115 |         await loadProfilesForSidebar();
      116 |         expect(instance.prefetchPosts).toHaveBeenCalledWith('mentionChannel0');
    > 117 |         expect(instance.prefetchPosts).toHaveBeenCalledWith('mentionChannel1');
          |                                        ^
      118 |         expect(instance.prefetchPosts).toHaveBeenCalledTimes(3);
      119 | 
      120 |         await props.actions.prefetchChannelPosts();

      at Object.<anonymous> (components/data_prefetch/data_prefetch.test.tsx:117:40)

  ● /components/data_prefetch › Should call for new prefetchQueueObj channels on change of prop and cancel previous ones

    expect(jest.fn()).toHaveBeenCalledTimes(expected)

    Expected number of calls: 3
    Received number of calls: 2

      142 |         await props.actions.prefetchChannelPosts();
      143 |         await loadProfilesForSidebar();
    > 144 |         expect(instance.prefetchPosts).toHaveBeenCalledTimes(3);
          |                                        ^
      145 |         wrapper.setProps({
      146 |             prefetchQueueObj: {
      147 |                 1: ['mentionChannel5', 'mentionChannel6'],

      at Object.<anonymous> (components/data_prefetch/data_prefetch.test.tsx:144:40)


Test Suites: 1 failed, 513 passed, 514 total
Tests:       2 failed, 4126 passed, 4128 total
Snapshots:   1335 passed, 1335 total
Time:        522.733 s
Ran all test suites.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] test: `cross-env NODE_ICU_DATA=node_modules/full-icu TZ=Etc/UTC jest --maxWorkers=50%`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] test script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/archit-p/.npm/_logs/2020-09-18T06_57_13_639Z-debug.log
make: *** [Makefile:34: test] Error 1

I'm unsure whether this is related to my setup or the code..

Also after making my changes is there a way to run specific e2e tests (i.e. ones that are affected by the changes), since otherwise e2e tests are taking 15+ mins to run.

regarding the failing test, I'd say it is odd. Please review that you are using the right node version as this has given people a lot of problems.

for e2e you can run npm run cypress:open and from there select the spec/s you want to run.

It was indeed an issue with my Node version and downgrading to v10.xx fixed it! Also npm run cypress:open worked great, I could search and run specific specs!

thanks a lot @archit-p :)

Was this page helpful?
0 / 5 - 0 ratings