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

web: Upgrade Rest Hooks to v5 #1907

Merged
merged 1 commit into from
Feb 7, 2021
Merged

Conversation

ntucker
Copy link
Contributor

@ntucker ntucker commented Feb 1, 2021

What

Updating the RH upgrade pr with

  • latest master
  • two patch releases of rest hooks
  • some more type fixes

How

Was able to get docker running locally and do all the tests. (It's fun to run webapp in dev mode and use redux devtools too :)).

It seems to be working, but not sure how to run integration tests locally. If this only runs on your CI - is there a way I can look at the failures to help debug?

Pre-merge Checklist

  • Run integration tests
  • Publish Docker images

@ntucker
Copy link
Contributor Author

ntucker commented Feb 1, 2021

Looking into making compatibility with super old v2 line of estlint parser

@michel-tricot
Copy link
Contributor

@ntucker thank you!! Adding @jamakase to review. He is on PTO at the moment.

@ntucker
Copy link
Contributor Author

ntucker commented Feb 3, 2021

I'm still trying to figure out the eslint parser problem so no need to review yet.

@jamakase
Copy link
Contributor

jamakase commented Feb 3, 2021

@ntucker we can't currently merge your PR. I tried once and faced some problems during CI so I have to revert the changes.

There is a draft PR based on your changes that includes some fixes for problems I faced after increasing react-scripts and rest-hooks version.

@ntucker
Copy link
Contributor Author

ntucker commented Feb 3, 2021

Oh can you link that pr and issues?

@ntucker
Copy link
Contributor Author

ntucker commented Feb 3, 2021

My goal is to make this work with older versions of react-scripts and typescript. This will help others upgrade as well. 5.0.2 should have typescript 3.7+ support restored, but there's some kind of eslint parse issue with the 2-line from react-scripts that I'm trying to figure out.

@ntucker
Copy link
Contributor Author

ntucker commented Feb 3, 2021

The weird thing is, I can't reproduce this simply by running old react scripts version and the resource files from the project on its own. (Line 0: Parsing error: Cannot read property 'map' of undefined is known to be an issue with eslint-parser for some modern javascript/typescript things)

@ntucker
Copy link
Contributor Author

ntucker commented Feb 3, 2021

facebook/create-react-app#9515 is the tracking

@ntucker
Copy link
Contributor Author

ntucker commented Feb 3, 2021

Found out the issue has to do with babel runtime and this change: babel/babel#10853. Some import compatibility changes going on

@ntucker
Copy link
Contributor Author

ntucker commented Feb 3, 2021

Reverted all tooling upgrades, and instead hoisted @babel/runtime. It should all work now

@ntucker
Copy link
Contributor Author

ntucker commented Feb 3, 2021

reactive/data-client#513 should make upgrades run smoother for legacy tooling situations like this

@ntucker
Copy link
Contributor Author

ntucker commented Feb 4, 2021

Eliminated need to force new runtime version with release of 5.0.3. Build and dev run work

@jamakase
Copy link
Contributor

jamakase commented Feb 4, 2021

@ntucker Is it correct, that we do not need to update our runtime and you have some changes that address that problem within new rest-hooks released version?

@ntucker
Copy link
Contributor Author

ntucker commented Feb 4, 2021

Yes, this branch has now been updated to use the latest rest-hooks which has been tested to be backwards compatible with all the tooling including:

  • eslint-typescript-parser
  • typescript 3
  • babel/runtime

You can observe now in the package-lock.json that only rest-hooks packages were changed/added.

@ntucker
Copy link
Contributor Author

ntucker commented Feb 4, 2021

I have tested locally by both building a production and running dev mode (npm start) with the docker running and a small sample project. It's by no means comprehensive but ensures that

  1. build is not broken
  2. The most common flows work correctly

@jamakase
Copy link
Contributor

jamakase commented Feb 4, 2021

Wow! @ntucker Thank you so much for making sure that upgrade will go fine. That is really awesome as I am pretty packed for the next sprint and was not sure when I have time to check the issue.

Let's try and merge it and see how CI goes. Just to make sure there are no lint errors anymore? As actually the problem we had was that yarn lint was failing for that case with map.

Copy link
Contributor

@jamakase jamakase left a comment

Choose a reason for hiding this comment

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

Thank's a lot for all the work! I understand that it could have taken a significant amount of time as you have to change some things not only here, but in rest-hooks lib too.

@ntucker
Copy link
Contributor Author

ntucker commented Feb 4, 2021

ntucker@GLaDOS:~/src/airbyte/airbyte-webapp$ npm run lint

> [email protected] lint /home/ntucker/src/airbyte/airbyte-webapp
> eslint --ext js,ts,tsx src

Lint passes. Oddly enough the linting issue was related to @babel/runtime being installed as a nested dependency. (Joys of packaging)

@ntucker
Copy link
Contributor Author

ntucker commented Feb 4, 2021

I tend to obsessively keep packages up to date, so this was a good opportunity to ensure smooth upgrades for all users of Rest Hooks. I suspect many people are stuck on react-scripts v3 as well.

@jamakase jamakase merged commit 5dfd81a into airbytehq:master Feb 7, 2021
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.

3 participants