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

Huge refacto and updates ! #19

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Huge refacto and updates ! #19

wants to merge 15 commits into from

Conversation

Djeg
Copy link
Contributor

@Djeg Djeg commented Mar 9, 2019

Hello folks !

Je viens de démarrer une série de refacto / mise à jour de ce superbe repo, pour répondre au besoin que nous avons sur "C'est quoi faire du react à KNPLabs ?" ou encore "Je suis nouveaux, je peux avoir un éxemple d'app React ?" ou bien "J'adore le doliprane, fait moi mal à la tête !".

L'idée principal est d'utiliser ce repo afin de proposer une architecture clair et mise à jour pour nos application React FRP (functional and reactive). Bien evidemment, personne n'est obligée d'utiliser ce repo pour leurs projets react (un simple create-react-app de facebook peut-être largement suffisant !). Cependant si vous chercher un point d'entrée pour une application plutôt bien grosse et que vous voulez vous mettre au "FRP" (Functional Reactive Programming) et bien ce repo est là pour ça :).

Qu'est-ce qu'il à bricoler encore le Jagon ?

1. Une nouvelle archi

Je propose cette architecture:

src/
  Component/ # Les composants react
  Effect/ # Toutes nos fonctions impure ! (Epic comprise dedans mais pas que ...)
  State/ # Nos modules Redux contenant Action / Reducer
  index.js # Le début des hostilités
  index.css # un "reset" css file

2. Une page d'accueil qu'elle est pas mal noir et blanc sexy

ReactAppScreenshot

Un peu de vocabulaire

Pour bien comprendre l'archi et tout le tintouin voici quelque notions expliqué vite fait et plutôt mal:

Pure / Impure

Une fonction est dite Pure lorsque qu'elle ne produit aucun effet secondaire. Un éxemple: const add = (x, y) => x + y. Attention toute mutations est considéré comme impure (ex: const pushCoucou = arr => arr.push('coucou') ici arr est muté et contient après l'appel 'coucou', la fonction est donc impure!)

Une fonction est dite impure lorsqu'elle contient au moins un petit riquiqui effet secondaire:

  • const currentHref = () => window.location.href Impure, car window.location.href est mutable!
  • const getUser = () => fetch('/users') Impure, car on ne peut pas prédire le résultat de la requète http

Les Effects

à chaque fois que vous rencontrer une fonction impure l'idée et de mettre ça dans le répertoire "Effect". Généralement la plupart de vos fonctions impure serons vos Epics, mais dans certains cas, un Observable est plutôt complétement overkill (comme lire la location). Pour ceci j'ai préinstallé crocks qui permet d'utiliser des "ADT" (technique répandu est fiable pour gérer des side effect).

Du coup un éxemple avec location.href serait:

// Without crocks (acceptable but could be better)
// currentHref :: forall a. () => a
export const currentHref = () => window.location.href

// With crocks (better, faster, stronger ...) (remarqué qu'on peut enlever le "a" dans la signature !
import { propPath } from 'crocks'
// currentHref :: () => Maybe String
export const currentHref = () => propPath(['location', 'href'], window)

À vous de choisir ce qui vous convient le mieux pour votre projet, aucune obligation d'utiliser crocks bien evidemment. Mais garder en tête qu'un effet est est simple fonction qui ne prend pas d'argument et fait de la magie:

// @type Effect a :: () -> a

// findUserByName :: String -> Effect (Promise User)
export const findUserByName = name => () => fetch(`/users/${name}`)

Component et State

Tout ce qui est dans Component ainsi que dans State doit obligatoirment être pure. Quelques exceptions faite pour les composant (dispatch et event sont autorisé bin sur).

Graçe à React 16.8 plus besoin de classe pour vos lifecycle event:

import React, { useEffect } from 'react'

const BookList = ({
  books = [],
  fetchBooksEff,
)} => {
  useEffect(fetchBooksEff, [true]);

  return (
   ....some JSX
 )
}

export default connect(
  state => ({ books: state.books }),
  dispatch => ({
    fetchBooksEff: () => dispatch(fetchBook())
 })
)(BookList)

Voili voilou, j'espère que ça vas vous plaire :). Hesitez pas à commenter à fond la caisse ! Des bisous !

@Djeg Djeg requested review from nicolasmure and NicolasNSSM Mar 9, 2019
Copy link
Contributor

@jaljo jaljo left a comment

Huge :) Thank you !

Styled Components
</Label>
Making design is often very painfull: How to organise classes ? How to make reusable components ?
Should i use id ? Does my classe works with this component ? Where is my fu***** stylesheet ? ...
Copy link
Contributor

@jaljo jaljo Mar 11, 2019

Choose a reason for hiding this comment

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

I :)

Making design is often very painfull: How to organise classes ? How to make reusable components ?
Should i use id ? Does my classe works with this component ? Where is my fu***** stylesheet ? ...
<br />
<br />
Copy link
Contributor

@jaljo jaljo Mar 11, 2019

Choose a reason for hiding this comment

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

Maybe It will be better to use p tags with style and not br ones, WDYT ?

Styled components is a very popular and handy tools allowing you make
designs with (wait for it...) Component.
<br/>
<br />
Copy link
Contributor

@jaljo jaljo Mar 11, 2019

Choose a reason for hiding this comment

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

Same here

</Fragment>


export const BlackStrip = styled.div`
Copy link
Contributor

@jaljo jaljo Mar 11, 2019

Choose a reason for hiding this comment

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

These styles should belong in ad edicated CSS file IMHO, to avoid very long files suche as this one.

Copy link
Contributor Author

@Djeg Djeg Mar 11, 2019

Choose a reason for hiding this comment

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

Ive play a lot with styled-components. I was first not confortable at all with the concept but gosh, it's sooooo more maintainable and brings css into your codebase directly!

I've say in the homepage:

        Making design is often very painfull: How to organise classes ? How to make reusable components ?
        Should i use id ? Does my classe works with this component ? Where is my fu***** stylesheet ? ...
        <br />
        <br />
        Styled components is a very popular and handy tools allowing you make
        designs with (wait for it...) Component.
        <br/>
        <br />
        You will love it!

Copy link
Contributor

@nicolasmure nicolasmure Mar 11, 2019

Choose a reason for hiding this comment

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

I agree too, if you still want to use styled-components, consider to put these styled components in a dedicated file ;)

Copy link
Contributor Author

@Djeg Djeg Mar 11, 2019

Choose a reason for hiding this comment

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

I don't agree. In many cases you may want to have a UI sort of file wich will contains some styled components for a given widget or anything, that's true. But here every component i have is directly attach to my domain. Why putting what is your domain outside of your domain ? you can still import those components import { BlackStrip } from './App' and we stay closest as possible to the domain :). I don't think it's needed here to separate. The application is a single page so ne no need plural files

Copy link
Contributor Author

@Djeg Djeg Mar 11, 2019

Choose a reason for hiding this comment

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

Besides, this repository is a bootstrapper, you will have to delete all this bootstrapping code. Here you just have to clean the App and start over. If i separate it will 2 files to delete. And i really don't see the point to separate here ?

Copy link
Member

@NicolasNSSM NicolasNSSM Mar 20, 2019

Choose a reason for hiding this comment

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

I'm not sure about all of this, didn't we said we shouldn't bloat this app? Maybe just a part in the main readme telling people styled components are great and show an example of how you can make it work within this very project could be way better?

expect(items.length).toBe(4)
});


Copy link
Contributor

@jaljo jaljo Mar 11, 2019

Choose a reason for hiding this comment

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

Why to have to charet return between each test ?

Copy link
Contributor Author

@Djeg Djeg Mar 11, 2019

Choose a reason for hiding this comment

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

SInce a function may contains "one chare return" inside there body, i find it more readable to separate them (i mean the functions) by two charet returns. Many people in the FP world are using this technic. It allows to put some charet return in functions as you want and separate this functions by two charret returns is a pleasure for the eye ^^

Copy link
Contributor

@nicolasmure nicolasmure left a comment

I liked the previous dog fetching example, it showed a common use case of resource fetching, state update and action reduction. Could be good to have such example here too ;)

Also, I think it'd be nice to introduce common app features, such as a router. We're currently refactoring the one of our frontend project using native history.pushState api and plain regexp with named captures for route patterns. If you want to check it out, see PR 371 of our frontend project (you know which one it is ;p ).
Defining some pages could be a good example IMO.

Nice landing page tho :D !

@@ -0,0 +1,3 @@
# Debug the application actions and state into the
Copy link
Contributor

@nicolasmure nicolasmure Mar 11, 2019

Choose a reason for hiding this comment

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

.env file should not be versionned (use the .env.dist file for that ;) ).
The app (no matter on which env) should use the .env file to compile and run. Considering this, the .env file should not be versionned and should be initialized from a deploy / init workflow (eg Makefile, CI).

Copy link
Contributor Author

@Djeg Djeg Mar 11, 2019

Choose a reason for hiding this comment

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

Why, since it's easier to not have the step of "Copy / paste" the env ?

Copy link
Contributor Author

@Djeg Djeg Mar 11, 2019

Choose a reason for hiding this comment

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

I mean, by versionning the .env we are winning:

  1. We don't need to copy / past the .env.dist file (just use a default .env file, it becames the .env.dist basicaly)
  2. Create react app comes with a .env.local override system, allowing you to do the same thing as you mentionned.

Copy link
Contributor

@nicolasmure nicolasmure Mar 11, 2019

Choose a reason for hiding this comment

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

Although overrides seems to be a quick win in the beginning, they end up in a mess in the mid / end of project development. As the .env file is the one used by the app, it should not be versionned as its values depends on which env the app is used.
Having a plain .dist file for each env (eg .env.dist for dev, .env.preprod.dist for preprod, .env.prod.dist for prod) is way cleaner and avoids possible overrides errors / misconceptions.

Having to copy / paste the dist file is not a big deal IMO as it is part of project initialization (same as deps install). It's still a one-liner :

$ make .env install-deps # executes both `.env` and `install-deps` targets

Copy link
Contributor Author

@Djeg Djeg Mar 11, 2019

Choose a reason for hiding this comment

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

Why does it mess up in the mid / end ?

Copy link
Contributor

@nicolasmure nicolasmure Mar 12, 2019

Choose a reason for hiding this comment

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

It gets harder to know what is the resolved value of a parameter for a given env when its value is split in multiple files. It's more readable and searchable to have a distinct env file per env with all the values in it.
Per se, the env file is dedicated to a single env, so versionning it binds it to multiple env and so is in contradiction with the aim of an env file.

@@ -11,7 +11,7 @@
src/**/*.css

# misc
.env
.env.local
Copy link
Contributor

@nicolasmure nicolasmure Mar 11, 2019

Choose a reason for hiding this comment

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

should be reverted (.env should not be versionned ;) ).

@@ -1,8 +1,5 @@
.env:
Copy link
Contributor

@nicolasmure nicolasmure Mar 11, 2019

Choose a reason for hiding this comment

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

I think this should be reverted too (see first comment ;) ).

</Fragment>


export const BlackStrip = styled.div`
Copy link
Contributor

@nicolasmure nicolasmure Mar 11, 2019

Choose a reason for hiding this comment

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

I agree too, if you still want to use styled-components, consider to put these styled components in a dedicated file ;)


// configureStore :: () -> Result Error Redux.Store
export const configureStore = tryCatch(() => {
const env = process.env.NODE_ENV || 'development'
Copy link
Contributor

@nicolasmure nicolasmure Mar 11, 2019

Choose a reason for hiding this comment

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

better fallback to prod env by default IMO (safer). dev env should be an opt-in config ;)

import ReactDOM from 'react-dom';
import './Style/Main.css';
import App from './Component/App';
import registerServiceWorker from './ServiceWorker';
Copy link
Contributor

@nicolasmure nicolasmure Mar 11, 2019

Choose a reason for hiding this comment

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

shouldn't this file be removed if not use anymore ?

Copy link
Contributor Author

@Djeg Djeg Mar 11, 2019

Choose a reason for hiding this comment

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

I think it's removed no ?

Copy link
Contributor

@nicolasmure nicolasmure Mar 11, 2019

Choose a reason for hiding this comment

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

oops my bad :p

return createStore(rootReducer)
}

const epicMiddleware = createEpicMiddleware()
Copy link
Contributor

@nicolasmure nicolasmure Mar 11, 2019

Choose a reason for hiding this comment

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

Could be nice to describe dependencies injection here ( https://redux-observable.js.org/docs/api/createEpicMiddleware.html ).
All the utilities used by the epics (such as fetch) should be injected as deps here as it will ease the tests of epics ;)

Copy link
Contributor Author

@Djeg Djeg Mar 11, 2019

Choose a reason for hiding this comment

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

Yep, why not. We need to write documentation (probably in a wiki). We can then add a section about thoses dependencies

@Djeg
Copy link
Contributor Author

@Djeg Djeg commented Mar 11, 2019

@NicolasNSSM i plan to make an entire example application, this repository must only be a starting point (like react does with create react app).

For the router, i personnaly recomand nothing :.p. Since it depends on the navigation in your app, in some cases you may not want to use history at all or you may want to use react router ... I don't think it's a good idea to put a complete routing system inside this project

@Djeg
Copy link
Contributor Author

@Djeg Djeg commented Mar 11, 2019

@nicolasmure my bad, i've just seen that i have pre-install react-router, my bad, i will remove this dependency

@nicolasmure
Copy link
Contributor

@nicolasmure nicolasmure commented Mar 11, 2019

What about also making a git tag of the current project state ?
If this PR gets merged, it's a different approach from the current state ;)

@NicolasNSSM
Copy link
Member

@NicolasNSSM NicolasNSSM commented Mar 20, 2019

On pourrait en profiter aussi pour mettre React Snapshot qui permet de prerender automatiquement toutes les pages publiques lors du build?

@Djeg
Copy link
Contributor Author

@Djeg Djeg commented Mar 22, 2019

@nicolasmure @NicolasNSSM @jaljo i'm wondering if we should not use : parcel instead of the complete "React create App" bootstrap. I've tried a bit to play with, it's realllllly simple ^^.

@NicolasNSSM good point about styled-components maybe we don't need it. So we should put everything inside an index.css for this landing page ?

An other point ... I've discover most wich is an other Observable library replacing Rx. It comes with many advantages such as:

  1. Compliant with fantasy-land wich allows us to directly use map, chain, filter from ramda in our observables.
  2. It's blasing fast! (no joking 3x times faster than Rx o_O)
  3. It respect the ES6 Observable RFC. Meaning that a Most Stream can be convert to an ES6 Observable easily and sent to Rx (i don't know why we should do that but anyway ^^)
  4. Since it's respect fantasy-land, it becomes much more easy to convert any crocks ADT into a most stream (ex: const asyncToStream = asyn => Most.fromPromise(asyn.toPromise()) or const maybeToStream = compose(asyncToStream, maybeToASync(Error('Nothing'))')

Last but not least, i've discover two little babel plugins very very usefull:

I would like to use them because it add sooooo much clarity in a Functional code flow ^^.

Let me know what you think about all of that :)

@NicolasNSSM
Copy link
Member

@NicolasNSSM NicolasNSSM commented Mar 22, 2019

Rhalala !
Je suis un peu réticient à l'idée d'augmenter encore la barrière à l'entrée technique d'un projet de boostrap, en revanche je suis carrément ok pour voir tout ça en action et pouvoir l'utiliser dans ce projet...
Ferions nous une branche "classique" (master) avec la base requise pour faire des dev' de qualitay et une autre un peu plus expérimentale avec toutes tes propositions ?

@Djeg Djeg mentioned this pull request Apr 5, 2019
@nicolasmure
Copy link
Contributor

@nicolasmure nicolasmure commented Apr 19, 2019

Indeed it could be good to have two distinct branches (eg master and next), especially for the new and experimental lib inclusions, while we don't have enough experience on it.

@NicolasNSSM
Copy link
Member

@NicolasNSSM NicolasNSSM commented Jul 3, 2019

What do we do with this PR?

@Djeg
Copy link
Contributor Author

@Djeg Djeg commented Jul 8, 2019

idk

...

I'm still thinking that actually we don't need this bootstrapper

Copy link
Member

@NicolasNSSM NicolasNSSM left a comment

.

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

4 participants