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

Upgrade react and react-formal #1930

Conversation

stefanhayden
Copy link
Collaborator

@stefanhayden stefanhayden commented Feb 19, 2021

Description

Our dependancies are way out of date. Some repos have been deprecated. Many have had many upgrades but have no CHANGELOG to guide how to upgrade.

Types of updates made:

  • react is a lot noisier about passing props that are not supported and so I've tried to only pass supported props where possible.
  • react-formal has a new way that inputs are passed in. No longer can we get a global type map and instead we need to use the as prop to pass in the component like <Form.Field as={GSTextField}
  • react-tap-event-plugin is deprecated and is said to no longer be an issue in browsers. So all onTouchTap are changed to onClick
  • yup has a new import pattern import * as yup from 'yup'
  • react expects key to be on components in arrays
  • react update stoped react-chartjs from working and it has been deprecated so I moved to react-chartjs-2 Updated chart.js requires a new data format passed in.
  • Form.Context no longer exists as used on ScriptList.jsx so components have been moved around. Its seems to function correctly now but react throws an error for having a form inside a form. The design might need to be altered to get rid of that error in the future.

Package Upgrades:

  • react ^15.6.1 -> 16.14.0
  • react-dom ^15.6.1 -> 16.14.0
  • react-formal ^0.20.0 -> 2.2.2
  • yup ^0.24.0 -> 0.32.3
  • enzyme ^3.3.0 -> ^3.11.0
  • react-formal ^0.20.0 -> 2.2.2
  • enzyme-adapter-react-15 ^1.0.5 -> DELETED
  • enzyme-adapter-react-16 ^1.15.6
  • react-hot-loader ^1.3.0 -> 4.13.0
  • react-tap-event-plugin ^2.0.0 -> DELETED
  • chart.js ^1.1.1 -> ^2.9.4
  • react-chartjs ^0.7.3 -> DELETED
  • react-chartjs-2 ^2.11.1
  • uglifyjs-webpack-plugin ^0.4.6 -> 1.3.0

Checklist:

  • I have manually tested my changes on desktop and mobile
  • The test suite passes locally with my changes
  • If my change is a UI change, I have attached a screenshot to the description section of this pull request
  • My change is 300 lines of code or less, or has a documented reason in the description why it’s longer
  • I have made any necessary changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • My PR is labeled [WIP] if it is in progress

@@ -25,7 +25,7 @@ import {
createStartedCampaign
} from "../../test_helpers";

describe("ConversationPreviewModal", async () => {
describe.skip("ConversationPreviewModal", async () => {
Copy link
Collaborator Author

@stefanhayden stefanhayden Feb 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently throws this error which I don't know how to fix:

Invariant Violation: Could not find "client" in the context of ApolloConsumer. Wrap the root component in an <ApolloProvider>.

      93 |       onRequestCloseMock = jest.fn();
      94 |
    > 95 |       component = mount(
         |                   ^
      96 |         <ApolloProvider client={ApolloClientSingleton}>
      97 |           <MuiThemeProvider>
      98 |             <ConversationPreviewModal

      at new InvariantError (node_modules/ts-invariant/lib/invariant.esm.js:12:28)
      at invariant (node_modules/ts-invariant/lib/invariant.esm.js:24:15)
      at node_modules/@apollo/react-common/lib/react-common.cjs.js:55:132
      at updateContextConsumer (node_modules/react-dom/cjs/react-dom.development.js:18304:19)
      at beginWork (node_modules/react-dom/cjs/react-dom.development.js:18661:14)

The error originates in the MessageResponse component at the line:

export default loadData({ mutations })(MessageResponse);

removing the loadData call like this lets the test pass:

export default MessageResponse

But I don't know what the issue is with loadData or how to solve it ATM.

@stefanhayden
Copy link
Collaborator Author

The updated react-chartjs-2 wrapper to chart.js library has a built in way to show labels.

image

@stefanhayden stefanhayden changed the title [WIP] Upgrade react and react-formal Upgrade react and react-formal Feb 26, 2021
@ibrand ibrand requested a review from schuyler1d March 3, 2021 19:10
@schuyler1d
Copy link
Collaborator

This is awesome work! Thank you! Running it locally, I haven't found any issues at all.
In trying to deploy it, I'm running into a bit of trouble.
First I needed to change .babelrc to:

{
  "presets": ["react", "env", "stage-0"],
  "only": ["*.js", "*.jsx"],
  "plugins": [
    ["transform-runtime", {
      "polyfill": false,
      "regenerator": true
    }]
  ],
  "env": {
    "dev": {
      "plugins":["react-hot-loader/babel"]
    }
  }
}

because hot-loader isn't available in dev dependencies.

Second, I'm getting this (or similar) Invalid Assignment error when building the client (running ASSETS_DIR=./build/client/assets ASSETS_MAP_FILE=assets.json ./node_modules/.bin/webpack --config ./webpack/config.js) -- I'm going to look at these closer over the next day or so, but if you get to it before me, then that would be very welcome.

@stefanhayden
Copy link
Collaborator Author

stefanhayden commented Mar 5, 2021

I was able to get ASSETS_DIR=./build/client/assets ASSETS_MAP_FILE=assets.json ./node_modules/.bin/webpack --config ./webpack/config.js to work by upgrading uglifyjs-webpack-plugin to 1.3.0. After 1.3.0 (which is 2.0.0) they require webpack 4.

I also added your .babelrc config change.

@stefanhayden
Copy link
Collaborator Author

stefanhayden commented Mar 10, 2021

it looks like the cypress tests are passing now too?

scratch that. I got over excited and was looking in the wrong place.

@stefanhayden
Copy link
Collaborator Author

OKAY! Now the cypress tests pass! https://github.com/stefanhayden/Spoke/actions/runs/637837685

@schuyler1d
Copy link
Collaborator

VERY EXCITING!! I've deployed your changes and can confirm that the Settings page is working now.

schuyler1d
schuyler1d previously approved these changes Mar 10, 2021
Copy link
Collaborator

@schuyler1d schuyler1d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work!

@schuyler1d schuyler1d added the S-ready for stage-main (qa) Status (ADMINS ONLY): PR label for those ready to be added for stage: Approved, tests, etc label Mar 10, 2021
@schuyler1d
Copy link
Collaborator

@stefanhayden we found one more issue with the auto-completes in CampaignTextingHoursForm.jsx -- the auto-completes don't work and the times don't show am/pm but just accept absolute number text values. Similarly setting Texting Hours in (organization) Settings only shows number values without autocomplete.

If I am able to work on this, I will update this PR, but if you have a chance to check it out, that would be wonderful.

@stefanhayden
Copy link
Collaborator Author

stefanhayden commented Mar 16, 2021

@schuyler1d Should be fixed now! Thanks for the amazing help with bug reports.

…lete w/ different labels/values is weird with new react version and current material-ui. does not feel much of a regression

Also added support for timezone range separate from hard-coding USA
@schuyler1d
Copy link
Collaborator

@stefanhayden That change didn't seem to fix it when I was testing -- current values weren't being shown and it didn't seem to trigger a save. After some futzing, I think there's some real issue with AutoComplete component mixed with the react-formal update when the labels differ from the values. So I have a commit that switches them to fixed menus here:
MoveOnOrg@2146886

If that works for you, then I can merge that in. Otherwise, if you want to try getting it to work with the autocomplete, lmk and you can try your hand at it.

schuyler1d and others added 2 commits March 18, 2021 21:35
…lete w/ different labels/values is weird with new react version and current material-ui. does not feel much of a regression

Also added support for timezone range separate from hard-coding USA
@stefanhayden
Copy link
Collaborator Author

@schuyler1d hm. I'm confused. I double checked what I had and it seemed to work for me. Here is a gif of what I see:

texting hours working

When I pulled your code down just opening the section throws this error: Uncaught TypeError: this.addFormField is not a function. But when I rename addAutocompleteFormField to addFormField then I see it also working but with the dropdown covering the page:

image

so I pulled that in.

@schuyler1d
Copy link
Collaborator

schuyler1d commented Mar 19, 2021

@schuyler1d hm. I'm confused. I double checked what I had and it seemed to work for me. Here is a gif of what I see:
It's not the timezone selection but if you choose 'override texting hours' and then the start/end-time hour autocompletes that fail

One more issue we've discovered (Haven't had time to investigate)

@stefanhayden
Copy link
Collaborator Author

hmmm. I'm not an expert is setting up graphql. It seems like useLazyQuery would let you call it conditionally but required hooks which our version of react-apollo doesn't have. I can try and upgrade it but I'm worried about causing more problems them I fix.

@stefanhayden
Copy link
Collaborator Author

stefanhayden commented Mar 23, 2021

On further investigation is seems that just wrapping that UserEdit in the is what cause it to die. if I remove it from the dialog it works. But I can't yet figure out what about the dialog kills it.

In my branch where I'm upgrading to material-ui 4 the new dialog doesn't have this problem. stefanhayden#1

@stefanhayden
Copy link
Collaborator Author

@schuyler1d Fixed the edit button not working. seems like any place with that kind of graphql error we can fix by just wrapping it with the provider again. It seems the old material ui dialog just does something to not pass the context along.

@schuyler1d
Copy link
Collaborator

ah ha! I pushed your change into load-data.jsx so we don't need to worry about it within components.
MoveOnOrg@06cd6e9

@schuyler1d
Copy link
Collaborator

This was merged (with some additional changes) into Spoke 10.1 release. Post-release a regression was reported in not being able to choose the rowsize in Campaigns and Message Review DataTables -- seems to be a bug/issue with material-ui/DropDownMenu. Reporting this here for tracking and for testing when e.g. material-ui is updated.

@stefanhayden
Copy link
Collaborator Author

thanks!

@schuyler1d
Copy link
Collaborator

And this was merged so I'm going to close! This was AMAZING work that's really helping us remove some technical debt. Thank you!!!!! (and onward to material-ui!)

@schuyler1d schuyler1d closed this Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-ready for stage-main (qa) Status (ADMINS ONLY): PR label for those ready to be added for stage: Approved, tests, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove dependences that use React.PropTypes
3 participants