Skip to content
This repository has been archived by the owner on Feb 26, 2021. It is now read-only.

React update all+move to functional components with hooks #277

Conversation

cvolant
Copy link

@cvolant cvolant commented Nov 6, 2019

  • Perform 'meteor update' and 'meteor npm update --all-packages'
  • Transform every class component into functional component
  • Fix not working menu
  • Fix unrecognized locale on app first load
  • Fix unwanted NotFound appearances while loading todos
  • Improve PropTypes

Locale and menuOpen are handled by simple global states provided through custom hooks built on useState and useContext.
Tests are broken... :/ The problem seems to come from enzyme-mounting components that use hooks.

Some meteor packages still need to be updated, like aldeed:collection2, but can cause problems.
GlobalStateProvider, LocaleProvider, useLocale, MenuOpenProvider, useMenuOpen
Use the menuOpen global state to manage the menu opening through a transition transform from a state to a reactive var
Call useMenuOpen though it does not need menuOpen because its children still need it though props.
Remove unnecessary imports, remove transitional menuOpen
I18n is broken on this page and NotFound, since inherited BaseComponent was in charge of it
Move Authpage content to children, remove all unnecessary menuOpen, and a few more
@CLAassistant
Copy link

CLAassistant commented Nov 6, 2019

CLA assistant check
All committers have signed the CLA.

imports/ui/state/MenuOpenState.jsx Outdated Show resolved Hide resolved
imports/ui/state/LocaleState.jsx Outdated Show resolved Hide resolved
imports/ui/pages/AuthPageSignIn.jsx Outdated Show resolved Hide resolved
imports/ui/pages/AuthPageJoin.jsx Show resolved Hide resolved
imports/ui/layouts/AppContent.jsx Outdated Show resolved Hide resolved
imports/ui/layouts/AppContent.jsx Outdated Show resolved Hide resolved
imports/ui/layouts/App.jsx Outdated Show resolved Hide resolved
imports/ui/layouts/App.jsx Outdated Show resolved Hide resolved
imports/ui/containers/ListPageContainer.jsx Show resolved Hide resolved
And extract i18n call following @HorusGoul suggestions
Update react-router-dom and a few other packages that had been left unupdated using npm-check-update
Remove all Redirect components
Transfer redirection from App component to AppContent component
Implement @HorusGoul solution using custom useUnmountedRef hook
@cvolant
Copy link
Author

cvolant commented Nov 11, 2019

After these last commits, tests are still broken but it's getting better. 3 of clients tests still don't pass I don't know why.
I also got a warning from eslint about dependency cycle in the api:

  • todo.js import list.js, and incompleteCountDenormalizer.js;
  • list.js import todo.js;
  • incompleteCountDenormalizer.js import list.js and todo.js.
    For now, I disabled the warning for these 3 files but it's not satisfactory. Any idea how to fix this?

@HorusGoul
Copy link

Just sent you a PR with some changes to solve these issues.

Regarding useTracker, we'll probably need to publish an update of this example once the hook gets released.

HorusGoul and others added 2 commits November 14, 2019 11:02
…nal-components-with-hooks

Ignore import/no-cycle rule and remove enzyme tests
@cvolant
Copy link
Author

cvolant commented Nov 14, 2019

Anything else needs some changes?

@HorusGoul
Copy link

Nope, I think we can already merge this, thank you for your collaboration 🎆

@thanateros
Copy link

I also started the todo react example using hooks from the beginning and ran into troubles when adding the withTracker code.

I came across this issue. Is what I'm reading here about making Metor work with hooks, or just the react todo example?

@cvolant
Copy link
Author

cvolant commented Nov 19, 2019

@thanateros Meteor does work with hooks, and it doesn't conflict with withTracker: this PR is about updating the todo example and make it use hooks. You can have a look at the code to see how it achieves that.

withTracker has also been rewritten as a useTracker hook, but it hasn't been published yet: it is currently worked on.

@thanateros
Copy link

@cvolant Thanks for the explanation.

@filipenevola filipenevola merged commit feef863 into meteor:react Nov 19, 2019
@filipenevola
Copy link
Collaborator

filipenevola commented Nov 19, 2019

Great, thanks @cvolant and @HorusGoul 🎉

@cvolant what about you making a tweet about this update and then we can RT?

@cvolant
Copy link
Author

cvolant commented Nov 20, 2019

Good idea. But I don’t have Twitter... 🙃 Nor Facebook.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants