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

Refactor/dependencies #981

Merged
merged 16 commits into from
Jun 19, 2020
Merged

Refactor/dependencies #981

merged 16 commits into from
Jun 19, 2020

Conversation

chrismclarke
Copy link
Member

@chrismclarke chrismclarke commented Jun 15, 2020

Follows #952 (will close/re-open after to see correct file diffs)

More core improvements, this time focused on our current dependency list

Main Changes

  • Add react-scripts. This is the original create-react-template that handles lots of build tooling behind the scenes. We had previously ejected to have more custom control when there used to be no support for sass, service workers etc. but since then the tooling has come a long way and reverting back to the default scripts provides a lot of simplification and stabilisation improvements.
  • Integrate react-rewired. This allows more flexibility and custom tooling with react scripts, specifically to add our custom improvements to the default service worker which had previously been nested deeply within workpack scripts (harder for devs to use)
  • Upgrade react to latest version. v16.8 -> v16.13. This hadn't been easy to do previously because of managing lots of related dependencies in ejected app, but now a much smoother process. Should not have any breaking changes.
  • Remove unused dependencies (either default included in react-scripts or previously added to the platform but no longer used)
  • Significant net code reduction

Additional changes

  • Various package upgrades
  • Swap tslint for eslint. Previously eslint didn't support typescript so we used a mix of both eslint (js/jsx) and tslint (ts/tsx) but now it does and is recommended a single solution (tslint is slowly being deprecated in favour or eslint).
  • Handle additional linting rules and minor type fixes
  • Continue to improve system that links different sets of dependencies together (e.g. functions with main src), ahead of eventual migration to lerna/yarn workspaces.

Breaking Changes

The large change in dependencies will mean likely devs will need to delete their node_modules folder and run yarn install again.

Recommended Testing

Have checked that main repo and functions build, as well as storybook and general tests. A few minor changes were made to component styles, however, these were only in terms of typing and for locationInput correcting a noStyle variable which wasn't correctly defined. A quick live test would be worth doing just to make sure things look generally as intended.

Todo

  • Update wiki
  • Refactor models to avoid import from rest of src (this makes cross-linking messier)
    (likely to be done as part of modular refactor/lerna integration so not creating issue for now)
  • Update pwa assets (manifest, logo, robots) ([feature request] - Enable better PWA support #982 )
  • Test pwa support (e.g. app icon shortcut) ([feature request] - Enable better PWA support #982 )
  • Include cypress in eslint (currently ignored) (not too important as only for tests and previously wasn't supported very well)
  • Upgrade functions packages (not creating issue as there will always be updates)
  • Upgrade further src packages (not creating issue as there will always be updates)

@chrismclarke chrismclarke requested a review from BenGamma June 15, 2020 02:26
@cypress
Copy link

cypress bot commented Jun 15, 2020



Test summary

60 0 0 0


Run details

Project onearmy-community-platform
Status Passed
Commit 27e65ad ℹ️
Started Jun 16, 2020 4:46 AM
Ended Jun 16, 2020 4:50 AM
Duration 04:42 💡
OS Linux Ubuntu Linux - 16.04
Browser Multiple

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@BenGamma
Copy link
Contributor

BenGamma commented Jun 19, 2020

Hey thanks for that one 💪

Everything works fine on my machine too, there is just some linting warnings in my console but nothing critical (mainly unsused-var).

Special big up for the yarn test command, I was always mixing the 2 (between the app in test mode and open Cypress panel 😅)

Will merge and give it an other round of tests from the staging env.

@BenGamma BenGamma merged commit a51665f into master Jun 19, 2020
@BenGamma BenGamma deleted the refactor/dependencies branch June 19, 2020 16:11
@BenGamma BenGamma mentioned this pull request Jul 13, 2020
@BenGamma BenGamma mentioned this pull request Sep 30, 2020
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.

2 participants