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

Update app to latest Node version #17

Merged
merged 6 commits into from Sep 18, 2018
Merged

Update app to latest Node version #17

merged 6 commits into from Sep 18, 2018

Conversation

c4lliope
Copy link
Contributor

This change includes several different commits;
walk through the commits individually for details on changes.

There is no net functional change in the app;
instead, this change improves the overal robustness of the app
and speeds up the developer feedback loop with automatic reloading.

Grace added 5 commits September 15, 2018 19:50
Node 10 and the base `create-react-app` configuration
has come a long way since this project was started.

The quickest way to take advantage of the new React and JS features
introduced since version 8 is to:

* Install Node 10
* Create a new, Node 10 React app "harness"
* Migrate the existing application code into the new application harness

Luckily, it's fairly easy to transplant the app's code
from an old project to a new, state of the art app.
This commit handles the first two steps in that process.

This fixes a number of hurdles that I encountered
when trying to install the `styled-components` library,
and generally results in fewer headaches.
These files are not referenced anywhere in the app.
The working app has been migrated to the new app harness,
and no references remain to these files.

They are dead code - likely imported and used in development,
without being cleaned up when their corresponding features were removed
from the app.

This is great - we slim down the size of our project considerably.
@c4lliope
Copy link
Contributor Author

Demos that:

  1. Functionality remains unchanged

functionality

  1. Development experience is improved

dev_loop

@c4lliope
Copy link
Contributor Author

c4lliope commented Sep 18, 2018

@pmanko Can you run through the following test script on your computer for me?

type node
type yarn
node -v
yarn -v

# in your local copy of the `tb-mobile-app` repo
git checkout uptodate

# This might break if your node version < 10
yarn
yarn start

A browser page should open for you.
If you make a change to src/App.js,
you should see the change appear in the UI.

Now, to test that it works properly in Docker: (Make sure to replace the paths!)

cd ~/path_to/tb-api

git fetch
git checkout client-dev
git pull
echo "COMPOSE_FILE=docker-compose.yml:docker-compose.dev.yml" >> .env
echo "TB_MOBILE_CONTEXT=~/path_to/tb-mobile-app" >> .env
docker-compose up -d

Click around the app, make sure everything works.

If you change src/App.js again, you should see the page pick up the changes.

Let me know how it looks! Thanks.

Copy link
Member

@ivan-c ivan-c left a comment

Choose a reason for hiding this comment

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

Works just as expected, except text and icons "vibrate" when browser window is resized vertically on login screen or when clicking buttons on /info.

Not sure if that's the new intended behavior, but don't bother to change if too difficult or would be easier to address in follow-up PR

import ReactDOM from 'react-dom';
import App from './App';

it('renders without crashing', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Are the tests passing? How is the test suite started? (thinking about setting up some CI)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, all the tests are failing.

I haven't looked at the test cases much, but I'd like to get them running again.
I'll take a look when I start moving the logout button.

You should be able to run yarn test; you'll get an interactive command-line test runner.

Any preferences for a CI platform? I can start setting that up.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation, glad to hear you have a rough plan to get tests working again.

Any preferences for a CI platform? I can start setting that up.

I have the most experience with TravisCI, and found it pretty flexible (root access and docker support and great free plan for FOSS), but I'm not attached if there's something you're more comfortable with.

@ivan-c
Copy link
Member

ivan-c commented Sep 18, 2018

Wanted to note I saw this warning when building the front-end docker image:

(node:8) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.

@c4lliope
Copy link
Contributor Author

(node:8) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.

Ah right, I saw that too.
It looks like this is coming from yarn, and was fixed in a more recent version.

I'll see if I can upgrade our yarn version.

@pmanko pmanko merged commit 37b343a into master Sep 18, 2018
@ivan-c ivan-c deleted the uptodate branch September 18, 2018 22:19
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.

None yet

3 participants