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.

Sign up for GitHub

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

Jump to bottom

cmd, dashboard: use webpack dev server, remove custom assets #16263

Merged
merged 2 commits into from
Mar 8, 2018
Merged

cmd, dashboard: use webpack dev server, remove custom assets #16263

merged 2 commits into from
Mar 8, 2018

Conversation

kurkomisi
Copy link
Contributor

@kurkomisi kurkomisi commented Mar 6, 2018 •
edited
Loading

  • use yarn instead of npm (comparison)
  • use webpack-dev-server instead of geth serving custom resources
  • throw out Assets config from geth
  • change KB to KiB in CustomTooltip (explanation)
  • change message structure
  • change chart tooltip descriptions meaningfully
  • use link to commit hash
  • use intuitive menu icon
  • define yarn scripts

karalabe
karalabe reviewed Mar 6, 2018
View reviewed changes
.gitignore Outdated
@@ -42,3 +42,5 @@ profile.cov
/dashboard/assets/node_modules
/dashboard/assets/stats.json
/dashboard/assets/bundle.js
/dashboard/assets/yarn-error.log
/dashboard/assets/package-lock.json
Copy link
Member

Choose a reason for hiding this comment

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

You want package lock committed to the repo. That's what ensures reproducible builds.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, there's yarn.lock. sry

karalabe
karalabe reviewed Mar 7, 2018
View reviewed changes
dashboard/README.md Outdated
$ (cd dashboard/assets && ./node_modules/.bin/flow-typed install)
```

Normally the dashboard assets are bundled into Geth via `go-bindata` to avoid external dependencies. Rebuilding Geth after each UI modification however is not feasible from a developer perspective. Instead, we can run `webpack` in watch mode to automatically rebundle the UI, and ask `geth` to use external assets to not rely on compiled resources:
Normally the dashboard assets are bundled into Geth via `go-bindata` to avoid external dependencies. Rebuilding Geth after each UI modification however is not feasible from a developer perspective. Instead, we can run `webpack-dev-server` to run a `geth` independent server which automatically rebundles the UI, and uses external assets to make connection with `geth`, which this way does not rely on compiled resources:

Copy link
Member

Choose a reason for hiding this comment

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

Normally the dashboard assets are bundled into Geth via go-bindata to avoid external dependencies. Rebuilding Geth after each UI modification however is not feasible from a developer perspective. Instead, we can run yarn dev to watch for file system changes and refresh the browser automatically.

karalabe
karalabe reviewed Mar 7, 2018
View reviewed changes
dashboard/README.md Outdated
$ (cd dashboard/assets && ./node_modules/.bin/webpack --watch)
$ geth --dashboard --dashboard.assets=dashboard/assets --vmodule=dashboard=5
$ geth --dashboard --vmodule=dashboard=5
$ (cd dashboard/assets && ./node_modules/.bin/webpack-dev-server)
Copy link
Member

Choose a reason for hiding this comment

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

yarn dev

karalabe
karalabe reviewed Mar 7, 2018
View reviewed changes
dashboard/assets/components/Dashboard.jsx Outdated
// gethHost is the host of geth.
const gethHost = 'localhost';
// gethPort is the port of geth.
const gethPort = 8080;
Copy link
Member

Choose a reason for hiding this comment

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

Only do this for dev mode.

karalabe
karalabe reviewed Mar 7, 2018
View reviewed changes
dashboard/assets/components/Dashboard.jsx Outdated
@@ -178,7 +183,7 @@ class Dashboard extends Component<Props, State> {
// reconnect establishes a websocket connection with the server, listens for incoming messages
// and tries to reconnect on connection loss.
reconnect = () => {
const server = new WebSocket(`${((window.location.protocol === 'https:') ? 'wss://' : 'ws://') + window.location.host}/api`);
const server = new WebSocket(`${((window.location.protocol === 'https:') ? 'wss://' : 'ws://')}${gethHost}:${gethPort}/api`);
Copy link
Member

Choose a reason for hiding this comment

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

Only do this for dev mode.

karalabe
karalabe reviewed Mar 7, 2018
View reviewed changes
dashboard/assets/components/Dashboard.jsx Outdated
log: [],

version: null,
commit: null,
Copy link
Member

Choose a reason for hiding this comment

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

Let's try to keep the network message independent of UI layout.

karalabe
karalabe reviewed Mar 7, 2018
View reviewed changes
dashboard/assets/components/Footer.jsx Outdated
const TRAFFIC = 'traffic';

const TOP = 'Top';
const BOTTOM = 'Bottom';
Copy link
Member

Choose a reason for hiding this comment

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

pls align :P

@kurkomisi kurkomisi mentioned this pull request Mar 7, 2018
Merged
karalabe
karalabe approved these changes Mar 8, 2018
View reviewed changes
Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

LGTM!

@karalabe karalabe merged commit 704840a into ethereum:master Mar 8, 2018
prestonvanloon pushed a commit to prestonvanloon/go-ethereum that referenced this pull request Apr 2, 2018
@kurkomisi @prestonvanloon
cmd, dashboard: use webpack dev server, remove custom assets (ethereu…
a69e322 
…m#16263)

* cmd, dashboard: remove custom assets, webpack dev server

* dashboard: yarn commands, small fixes
hackmod pushed a commit to OpenCommunityCoin/go-esn that referenced this pull request Jul 9, 2018
@kurkomisi
cmd, dashboard: use webpack dev server, remove custom assets (ethereu…
bd98b1b 
…m#16263)

* cmd, dashboard: remove custom assets, webpack dev server

* dashboard: yarn commands, small fixes
mariameda pushed a commit to NiluPlatform/go-nilu that referenced this pull request Aug 23, 2018
@kurkomisi @mariameda
cmd, dashboard: use webpack dev server, remove custom assets (ethereu…
98473ad 
…m#16263)

* cmd, dashboard: remove custom assets, webpack dev server

* dashboard: yarn commands, small fixes
firmianavan pushed a commit to firmianavan/go-ethereum that referenced this pull request Aug 28, 2018
@kurkomisi @firmianavan
cmd, dashboard: use webpack dev server, remove custom assets (ethereu…
58a7c42 
…m#16263)

* cmd, dashboard: remove custom assets, webpack dev server

* dashboard: yarn commands, small fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karalabe karalabe karalabe approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kurkomisi @karalabe