Refactor using Webpack and JSX #84

HerveH44 · 2018-01-20T17:11:27Z

I'll try to explain the whole idea behing the "refactoring".

Apply JSX and ES6 on frontend

Be Heroku compliant => get rid of Makefile

The first goal was to use JSX and ES6 features for the frontend. It's mostly used as syntaxic sugar to ease the readibility of the code. JSX forced me to use Babel to transpile into to JS. The transpiler is embedded in Webpack. I think we could get more from Webpack but I couldn't figure out how to tell him to move some dependencies to public/lib, so it's now done by the "scripts/postinstall.js" which executes at the end of the command "npm install".

This also helped to get rid of the makefile. The main problem of the current application is that it needs to run "make run". This is not Heroku compliant (which requires the application to be rdy only doing "npm start" - "make" and "git" commands are not available at runtime). So I needed to make the app run only via JS.

I did then a bit of refactoring on the frontend : I dispatched the components into separate files when I thought it would make better sense. The components are as dumb as I could make them so it makes them more reusable.

I also did a bit of refactoring on the root directory, the default config are now in config/, scripts/ has the app, run and postinstall JS.

More:

The dockerfile is already inside yes. the other PR could be closed if this one is validated.

The Makefile is gone yes, now everything should be able to run through npm, if not we can modify the package-json "scripts" module. In fact, looking at the package-json and you should also see an info for the "contributors". I added mine but didn't know if you wanted and how to write your pseudo.

In addition, this includes several other changes:

Fixes host: possibility to change seating #23
Fixes Dockerfile #48 (and replaces Add Docker #56)
Fixes "suggest lands" not working #55
Fixes hide "Download Draft Log" button for any non-draft games #63
Fixes install instructions incomplete #76
Fixes missing separator dots in firefox #79
Fixes counting picks is the wrong way #81
Pre-work for setup Heroku Review Apps #66
Fixes Dev: Add possibility to change seating (#23) and possibility to shuffle #33
Fixes cmc of split+aftermath cards are wrong #86 : apply new rule of CMC for aftermath and split cards
Fixes move "sets to ignore" into a const at top #75 : move sets to ignore into a const at top
Fixes more information about public rooms #43
Fixes format wrong info + pack info missing #22 : display game info to players and lobby
e157bf

See a running version with this PR included: https://beta-dr4ft.herokuapp.com/

ZeldaZach

Great job, Herve! A few comments to address and I'll want @dev-id to look at it :)

ZeldaZach · 2018-01-20T18:34:10Z

config/config.server.js.default

-let versionInfo = execSync('git describe --always --long')
-  .toString()
-  .trim()
+let versionInfo = "noVersion"


Will this always say "noVersion" or will it get the git hash?

ZeldaZach · 2018-01-20T18:40:00Z

public/src/game/Chat.jsx

      messages: []
    }
-  },
+  }
+  // getInitialState() {


Any reason this is commented out?

ZeldaZach · 2018-01-20T18:40:06Z

public/src/game/Chat.jsx

  componentDidMount() {
-    this.refs.entry.getDOMNode().focus()
-    App.on('hear', this.hear)
+    // this.refs.entry.getDOMNode().focus()


Any reason this is commented out?

ZeldaZach · 2018-01-20T18:43:29Z

src/pool.js

@@ -92,7 +92,6 @@ function toPack(code) {
    case 'MMA':
    case 'MM2':
    case 'MM3':
-    case 'IMA':


Is Iconic masters not a specialty set?

tooomm · 2018-01-23T22:24:49Z

Will this include #56? I saw a Dockerfile in here, too. :)
Will this include all fixes (lands box etc.) and additions (change copy&paste field etc.) besides the actual refactor? (linking #82)

I see the Makefile is removed too?
So this sets us up for Heroku Review Apps as well?

HerveH44 · 2018-01-24T07:51:03Z

Hey Tom,

The dockerfile is already inside yes. the other PR could be closed if this one is validated.

I don't know exactly what you mean by fixes on lands, box ... but this branch is deployed on https://beta-dr4ft.herokuapp.com/ is you want to get and check if there are any bugs

The Makefile is gone yes, now everything should be able to run through npm, if not we can modify the package-json "scripts" module. In fact, looking at the package-json and you should also see an info for the "contributors". I added mine but didn't know if you wanted and how to write your pseudo.

The app, as you can see, is up for heroku review apps

tooomm · 2018-01-24T13:04:56Z

Sounds great.

I don't know exactly what you mean by fixes on lands, box ... but this branch is deployed on https://beta-dr4ft.herokuapp.com/ is you want to get and check if there are any bugs

When you worked on the refactor initially, we had a lot of chats where I pointed bugs out etc.
Some related to your refactor and others had been there before.
You fixed all of them except some chat or merging of different game-states issue, if I recall correctly.
The Suggest land box is one example, it doesn't always work on dr4ft right now, but does in your linked heroku app. 👍

Since we have no PR description and overview what changed in addition to the refactor itself everything is a bit unclear. I'm pretty sure several issues can be closed once this is merged. While I can double check and take care of that, it would be helpful for reviewing and documenting changes in general, to have a rough list of bullet points that you addressed on top of the general code changes needed to use BabelJS for example.

Like:

 - added Docker support
 - removed the need of Makefile (all runs trough npm + package-json --> Heroku Review App support)
 - fix "Suggest land box"
 - ...

Because everything is in one commit it's really difficult to follow, reconstruct and review!

Everybody looking at this PR (now, or in month because a bug got introduced for example) just sees 10k changed lines and has no clue what went on or what to watch out for. :)

Can you please try to summarize things a bit in the description?

I hope @dev-id can review it sooner than later as well - he is the only one with deeper knowledge of the code/project at all.

HerveH44 · 2018-01-24T18:26:40Z

Thanks @tooomm for your post, that's clear that I have to make a summary of the changes, as they are quite unreadable from this enormous blob.

I'll try to explain the whole idea behing the "refactoring".

Apply JSX and ES6 on frontend
Be Heroku compliant => get rid of Makefile

The first goal was to use JSX and ES6 features for the frontend. It's mostly used as syntaxic sugar to ease the readibility of the code. JSX forced me to use Babel to transpile into to JS. The transpiler is embedded in Webpack. I think we could get more from Webpack but I couldn't figure out how to tell him to move some dependencies to public/lib, so it's now done by the "scripts/postinstall.js" which executes at the end of the command "npm install".

This also helped to get rid of the makefile. The main problem of the current application is that it needs to run "make run". This is not Heroku compliant (which requires the application to be rdy only doing "npm start" - "make" and "git" commands are not available at runtime). So I needed to make the app run only via JS.

I did then a bit of refactoring on the frontend : I dispatched the components into separate files when I thought it would make better sense. The components are as dumb as I could make them so it makes them more reusable.

I also did a bit of refactoring on the root directory, the default config are now in config/, scripts/ has the app, run and postinstall JS.

I think that's mostly it for the main picture. About the issues, i'm gonna quick check the ones that should be closed with this PR:
#23
#48
#55 I think
#63
#66
#76

Thank you for your time!
Herve

ZeldaZach · 2018-01-24T18:36:32Z

I'm good with rolling this in and establishing a new baseline

HerveH44 · 2018-02-04T10:52:59Z

Hey,

I just pushed some fixes for a new bug Tom encountered: the flipped card were broken because the number we were checking (number) is now wrong. We have to check the "mciNumber" to know if the card is the flip instance or the normal instance.

I also corrected the "error" field, which wasn't correct and deleted the useless TODO from frontend

…obby

ZeldaZach · 2018-02-12T04:27:28Z

Hey @HerveH44 Do you think we should merge this into master at this point? It's been long enough and there are a lot of changes here that would benefit the community

HerveH44 · 2018-02-12T08:06:16Z

Hi @ZeldaZach , I would prefer to have some beta testing first. @tooomm made some great reviews couple weeks ago. I was thinking if it would be possible for Cockatrice players to accept to beta-test on the heroku app (https://beta-dr4ft.herokuapp.com/) for a week or 2?

What do you think? We could also merge it and answer the possible bugs as they go. I start to have a bit more time this month so I could be reactive if issues arrive.

tooomm · 2018-02-12T09:12:02Z

There is no real point in merging as long as @dev-id isn't around to update the code on the server anyway?

Dave has a beta page himself which should be faster than heroku and can take some more load. If we could put it there and let people know we might get some tests and feedback!

HerveH44 · 2018-02-12T11:34:11Z

Ok, So we need to reach @dev-id first. @tooomm could you check the new fixes ?

Fixes #86 : apply new rule of CMC for aftermath and split cards

I updated the cmc of cards with layout aftermath. I checked cards from the original issue and it worked. But I don't know if I covered all possible layouts. Do you know where are the rules about it? The webpage wich was in the issue wasn't super clear about which cards type (aftermath, split or else...) where targeted by the new rule.

Fixes #75 : move sets to ignore into a const at top

I guess it's not possible to test...

Fixes #43 and #22: display game info to players and lobby

I changed the panel "Start Game" to "Game", which always displays "Format" and "Info" about the format (draft, sealed, cube...) and Info (XLN/XLN/XLN, or Cube 3x15 for example). I also added these informations in the "Join Panel" of the Lobby. I did some testing, but I forgot to check the txt filename which also use the same infos.

HerveH44 added 2 commits January 20, 2018 18:09

Refactor using Webpack and JSX

cd6ce9b

Fix pick order and refactoring

9b8522f

ZeldaZach requested changes Jan 20, 2018

View reviewed changes

HerveH44 added 2 commits January 20, 2018 20:24

fix: git version and delete commenting code

39527df

fix git version

3220607

ZeldaZach approved these changes Jan 20, 2018

View reviewed changes

tooomm mentioned this pull request Feb 1, 2018

counting picks is the wrong way #81

Closed

HerveH44 added 4 commits February 1, 2018 19:23

Fixes dr4fters#33 : delete 'cards' mention from zonehelper

88e74f8

fix Error displayfield

8462106

fix: fixing double-faced card from mciNumber

cd761da

ref: delete frontend TODO

1fca8f4

HerveH44 added 3 commits February 11, 2018 21:51

Fixes dr4fters#86 : apply new rule of CMC for aftermath and split cards

d650f8e

Fixes dr4fters#75: move sets to ignore into a const at top

d08ea59

Fixes dr4fters#43 and dr4fters#22: display game info to players and l…

e157bf2

…obby

ZeldaZach merged commit 1dbbcb1 into dr4fters:master Feb 19, 2018

HerveH44 mentioned this pull request Feb 19, 2018

Add Docker #56

Closed

HerveH44 deleted the refactor_new branch February 22, 2018 19:12

HerveH44 mentioned this pull request Feb 27, 2018

add info about game type/packs to room #44

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor using Webpack and JSX #84

Refactor using Webpack and JSX #84

HerveH44 commented Jan 20, 2018 •

edited by ZeldaZach

Loading

ZeldaZach left a comment

ZeldaZach Jan 20, 2018

ZeldaZach Jan 20, 2018

ZeldaZach Jan 20, 2018

ZeldaZach Jan 20, 2018

tooomm commented Jan 23, 2018 •

edited

Loading

HerveH44 commented Jan 24, 2018 •

edited

Loading

tooomm commented Jan 24, 2018 •

edited

Loading

HerveH44 commented Jan 24, 2018

ZeldaZach commented Jan 24, 2018

HerveH44 commented Feb 4, 2018

ZeldaZach commented Feb 12, 2018

HerveH44 commented Feb 12, 2018

tooomm commented Feb 12, 2018

HerveH44 commented Feb 12, 2018

Refactor using Webpack and JSX #84

Refactor using Webpack and JSX #84

Conversation

HerveH44 commented Jan 20, 2018 • edited by ZeldaZach Loading

ZeldaZach left a comment

Choose a reason for hiding this comment

ZeldaZach Jan 20, 2018

Choose a reason for hiding this comment

ZeldaZach Jan 20, 2018

Choose a reason for hiding this comment

ZeldaZach Jan 20, 2018

Choose a reason for hiding this comment

ZeldaZach Jan 20, 2018

Choose a reason for hiding this comment

tooomm commented Jan 23, 2018 • edited Loading

HerveH44 commented Jan 24, 2018 • edited Loading

tooomm commented Jan 24, 2018 • edited Loading

HerveH44 commented Jan 24, 2018

ZeldaZach commented Jan 24, 2018

HerveH44 commented Feb 4, 2018

ZeldaZach commented Feb 12, 2018

HerveH44 commented Feb 12, 2018

tooomm commented Feb 12, 2018

HerveH44 commented Feb 12, 2018

HerveH44 commented Jan 20, 2018 •

edited by ZeldaZach

Loading

tooomm commented Jan 23, 2018 •

edited

Loading

HerveH44 commented Jan 24, 2018 •

edited

Loading

tooomm commented Jan 24, 2018 •

edited

Loading