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

Add React and an example component #790

Merged
merged 8 commits into from
Dec 2, 2018
Merged

Add React and an example component #790

merged 8 commits into from
Dec 2, 2018

Conversation

nicksellen
Copy link
Contributor

Proposed Changes

  • add react just to see how it works/feels

Testing Instructions

  • run up as normal
  • look on profile page
  • the "Member since..." and "Online..." boxes on the left are rendered with a react component!

It's just a proof of concept to see how it integrates. That particular component has a load of other stuff too, and maybe not worth switching it over, but yeah, just gives an example. Seems quite nice!

@nicksellen nicksellen changed the base branch from webpack to master October 30, 2018 22:41
@mrkvon
Copy link
Contributor

mrkvon commented Nov 2, 2018

Exciting! Is this something to consider or use in the new client PRs already? Is the idea of using React (instead|on top) of AngularJS mature?

@simison
Copy link
Contributor

simison commented Nov 3, 2018

Is the idea of using React (instead|on top) of AngularJS mature?

Kinda as this PR well proves; that said, there are still unknowns regarding routing and data fetching — especially the latter one will be important to figure out sooner than later but we might be able to work our way around it for a while by using Angular app as an app-shell and limiting react only to components inside the app, basically having tons of render-trees all over the place.

Is this something to consider or use in the new client PRs already?

Definitely. It would be good to get some "framework" for doing this in place first with tiny changes like this.

References list could be an interesting experiment to try with React indeed, but only if it doesn't significantly slow us down shipping references.

@nicksellen
Copy link
Contributor Author

I added a import helper so all react components are automatically imported, from the comment in main.js:

/*
 *  Imports all react components from modules/ and register them as angular components
 *
 *  So if you defined modules/users/client/components/TrustrootsGreeting.component.js as:
 *
 *    export default function TrustrootsGreeting({ name }) {
 *      return <p>Hello {name}, from Trustroots!</p>;
 *    };
 *
 *  It will be available to use in (any) angular  as:
 *
 *    <trustroots-greeting name="'Donald'"></trustroots-greeting>
 *
 *  Uses a webpack require context
 *  See https://webpack.js.org/guides/dependency-management/#require-context
 */

@nicksellen nicksellen force-pushed the react branch 2 times, most recently from c0b8bba to 0987046 Compare November 3, 2018 12:56
@simison
Copy link
Contributor

simison commented Nov 3, 2018

I added a import helper so all react components are automatically imported, from the comment in main.js:

Good idea, although the more explicit imports we can have (even at module level) and less magic around, the clearer dependencies are going to be and easier everything is to grok.

This time convention over configuration (that the modules/users/client/config/users.client.config.js file is) totally makes sense though.

How do you see the path forward later on when we might have components that are very module (or "section") specific, as opposed to being more generic and re-usable?

At what point we could have completely explicit imports and remove importComponents?

@nicksellen
Copy link
Contributor Author

At what point we could have completely explicit imports and remove importComponents?

In the future :)

I guess the magic component imports are for using them within angular templates, any other use should be explicit import (and will have to be anyway). Doesn't seem much benefit to rewrite all the global style angular files into nice es6 modules with imports just to ditch them later.

Could find a way to rewrite into es6 modules + react at the route level in future perhaps, so one route goes to one react component. A few other patterns to work out though around data fetching, state, etc...

@nicksellen
Copy link
Contributor Author

Tests weren't working before when using react2angular, but ngreact works fine too (and has more github stars) and was easy to swap in.

I would suggest this is ready, except the example component I created is not complete, or very useful. but also probably not a good idea to merge it without any react components in use, so perhaps either:

  • @mrkvon or someone else wants to start building a real react component and can build on top of this branch (I would favour this option)
  • someone picks a really simple component to implement and we can merge with that one

Both cases would involve removing the one I created for this proof of concept.

@nicksellen
Copy link
Contributor Author

Also, https://medium.com/@jrwebdev/migrating-an-angular-1-application-to-react-8891ec73d462 has some thoughts, I only skim read it.

I would also suggest to keep react components simple and not try and get too much interaction between react and angular as I think it could quickly get messy, so when in react land I would try and avoid two way bindings, much/any interaction with the angular controller, using angular services, etc...

So probably don't try and pass whole controller objects through to a react component, but just the information that it needs.

@nicksellen
Copy link
Contributor Author

... ok, well I tried out the second option actually, and implement the volunteering page in react, which involved:

  • using a react component for a route (basically just template: '<volunteering></volunteering>)
  • emitting an event on the application scope (by creating the photo service)

I also added fancy es2018 object spread plugin. It's quite easy to start dropping browser support with some of these things if we don't get the babel options right (e.g. Object.assign() is not supported in ie11).

@simison simison self-requested a review November 7, 2018 21:16
@mrkvon
Copy link
Contributor

mrkvon commented Nov 10, 2018

@nicksellen How shall we talk with api? Within angular, or react? Do you have any guidelines to share? I'm interesting in both reading and writing data.

I'm very new to react, done the tutorial and bits of docs only. Also read a nice article about data fetching, which seems to assume full react application.

What is the long-term future picture of frontend? Everything i.e. react + redux? @nicksellen @simison

@nicksellen
Copy link
Contributor Author

Personally, I would keep it simple/incremental. Try with just fetch (+ polyfill), and if that doesn't have enough features then axios. Basically like that second link.

Although I would suggest to add one additional layer - an API service that abstracts any of the details about using fetch/axios. So inside a component you can use it something like:

// login
await api.auth.login({ username, password })
// fetch contacts
const contacts = await api.contacts.list()

That kind of thing...

At some point it might make sense to use redux, but probably not worth it until it seems useful. The API service will still be used then, as will the components.

If the presentation is getting too mixed up with logic you can always write two components:

  • one stateful component that does API calls, or whatever, passes down props and callback handler functions (e.g. onAddContact)
  • one that purely does presentation, renders only based on props, and calls handler functions to do stuff

@mrkvon
Copy link
Contributor

mrkvon commented Nov 14, 2018

I'd like to create a link (like ui-sref) from react component; maybe a redirect. I've been struggling with this most of the day. Most recently trying with @uirouter/react-hybrid, facing errors.

@nicksellen @simison Do you have any experience with wiring up routing between react and angular-ui-router? Or ideas?

I'm thinking about using href and forcing a page reload... 😒

@simison
Copy link
Contributor

simison commented Nov 14, 2018

I'm thinking about using href and forcing a page reload...

Yeah, depending on where these are it should be safe and best we can have when living with two frameworks.
E.g. moving between tabs on profile page using href can be problematic, but moving from "module to module" it quite safe.
Basically we'll probably hit some issues where we've used "sub views" of ui-router, but I think that's mostly profile pages and maybe modals.

@mrkvon
Copy link
Contributor

mrkvon commented Nov 17, 2018

rebased with master

@nicksellen
Copy link
Contributor Author

nicksellen commented Nov 19, 2018

@mrkvon you can access all the angular services from outside angular, e.g.:

var $state = angular.element(document).injector().get('$state');
// Go to messages page ("inbox" state)
$state.go('inbox')
// Get URL for messages page
$state.href('inbox')
// ... or one with params
$state.href('profile', { username: 'peter' })

And there is an example where I use an angular scope outside of angular in photos.services.js (to call $emit).

If you do anything more fancy outside of angular you might need to consider the digest scope to ensure things update in the angular world, but I think it should be ok for these uses.

@guaka guaka removed the in progress label Nov 21, 2018
@simison simison changed the title WIP: React Proof of Concept Add React and an example component Dec 2, 2018
This was recently removed when AddThis script was removed with invite-page.
Copy link
Contributor

@simison simison left a comment

Choose a reason for hiding this comment

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

@mrkvon feel free to merge once the about component is either part of the about -angular template or removed for now, since it doesn't implement most of the things for that section.

mrkvon added 2 commits December 2, 2018 22:54
to be part of the angularjs component, not to replace it
@mrkvon
Copy link
Contributor

mrkvon commented Dec 2, 2018

Rebased, moved the react profile-view-basics component and fixed a link.

I'll merge this now. It is the first merge I do into Trustroots master. Feel free to revert if there are any issues.
@simison @guaka

@mrkvon mrkvon merged commit b14f46a into master Dec 2, 2018
@mrkvon
Copy link
Contributor

mrkvon commented Dec 3, 2018

npm run build:prod is failing.

@mrkvon
Copy link
Contributor

mrkvon commented Dec 3, 2018

@nicksellen

It's quite easy to start dropping browser support with some of these things if we don't get the babel options right

Is this something you took care of, or is it yet to be done?

mrkvon added a commit that referenced this pull request Dec 4, 2018
guaka pushed a commit that referenced this pull request Dec 4, 2018
guaka pushed a commit that referenced this pull request Dec 4, 2018
This reverts commit b14f46a. - the React stuff that breaks production build
@guaka guaka mentioned this pull request Dec 4, 2018
mrkvon pushed a commit that referenced this pull request Dec 7, 2018
Merging back the react proof of concept #790. + fixes

* Make the `npm run start:prod` pass.

also
- add react version to .eslintrc
- make dependency versioning in package.json consistent (^ => ~)

* remove firebase from devDependencies (let's do this change in different PR)
@mrkvon mrkvon added the React label Dec 8, 2018
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.

4 participants