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

Web dev improvements #4388

Merged
merged 8 commits into from
Dec 5, 2017
Merged

Web dev improvements #4388

merged 8 commits into from
Dec 5, 2017

Conversation

edmundoa
Copy link
Contributor

@edmundoa edmundoa commented Dec 1, 2017

In the last few weeks we did quite a few changes in the development toolchain we use in the web interface, resulting in unfinished configurations, confusing scripts running different tools, and outdated documentation (or, let's say, even useless documentation).

This PR fixes at least some of those problems by:

  • Letting users customise the port of the development server
  • Use a random port when the first we try to use is taken (cc @lilliCao)
  • Remove alternative dev setup introduced in Add another way to run the development setup #4294, since we already fixed the issues with the dev-server that lead to its creation
  • Remove dev setup using react-hot-loader, as I could not find a way to fix it, and nobody seems to use it anyway. I wrote a bit more about the reasoning for this in the commit message. This change require some changes in some plugins as well. I will create PRs for them as well shortly
  • Due to the previous change, the dev setup not using react-hot-loader (start-nohmr) is now used in yarn start, as that is the most conventional script name to run an application
  • Update README file to include all these changes, the use of Yarn by default, and remove some old comments from there

I hope all this changes help making the setup easier to understand and clearer.

@edmundoa edmundoa added this to the 3.0.0 milestone Dec 1, 2017
@ghost ghost assigned edmundoa Dec 1, 2017
@edmundoa edmundoa removed their assignment Dec 1, 2017
@ghost ghost assigned edmundoa Dec 1, 2017
@edmundoa edmundoa removed their assignment Dec 1, 2017
@bernd bernd self-assigned this Dec 4, 2017
Edmundo Alvarez added 8 commits December 5, 2017 11:33
Let users customize port number by passing `--port=<port>` as argument.
If the port is already taken, we will start the web interface in a
random one.
Changed it accidentally during some tests.
The `start` build using HMR alongside react-hot-loader was already not
working as it should with webpack-dev-server. Modules got reloaded, but the
application state was reset on every change.

I have been trying a couple of times to make HMR work with the new express
dev server, but I just managed to reproduce what it achieved in the
previous build (reloading the whole component tree, but losing the state).
All of that making the current webpack configuration more complicated.

I think react-hot-loader is a good idea that can be helpful specially when
styling components, but currently I see that it produces way too maintenance
burden for the benefits it provides. Also, nobody in the team seems to be
using it for development.

Long story short, this commit removes react-hot-loader from our dev build. We
still use Webpack HMR to reload the browser.

The commit also replaces the `start` script with `start-nohmr`, so
running `yarn start` will use the no react-hot-loader dev server.
- Include details to use yarn in development
- Remove old warnings and information
- Remove mentions of the react-hot-loader build
- Update information on upgrade packages
- Update information on IntelliJ setup
@edmundoa edmundoa force-pushed the web-dev-improvements branch from 84e9ff6 to acf9dea Compare December 5, 2017 10:38
@ghost ghost assigned edmundoa Dec 5, 2017
Copy link
Member

@bernd bernd left a comment

Choose a reason for hiding this comment

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

LGTM 👍

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