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

Update package.json #2017

Merged
merged 8 commits into from
Nov 23, 2016
Merged

Update package.json #2017

merged 8 commits into from
Nov 23, 2016

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Nov 18, 2016

The only two things I didn't update are:

  1. d3 to version 4, because they introduced a lot of changes - I think this update should be done in a separate PR where we also make sure all the elements are displayed correctly
  2. eslint together with eslint-config-airbnb, which are also pretty much behind - I think we should do this in a separate PR as well; I tried doing it now but I got a lot of lint warnings, so I assume either our lint configuration would need to be revised or we would need to do a bunch of small fixes across a lot of files

I used http://npm.click/ for libraries' version comparison.

@fbarl fbarl changed the title [WIP] Update package.json Update package.json Nov 18, 2016
@fbarl
Copy link
Contributor Author

fbarl commented Nov 21, 2016

@foot: Maybe you can take a look at this since @davkal is not here today, so that I can merge it soon :)

@foot
Copy link
Contributor

foot commented Nov 21, 2016

  • Hot reloading is not working (would be realllly nice to keep this working).

But most of the UI seems to be working fine.

@foot foot assigned fbarl and unassigned foot Nov 21, 2016
@davkal
Copy link
Contributor

davkal commented Nov 22, 2016

Hot reloading is not working (would be realllly nice to keep this working).

I'd say that's a hard requirement.
Maybe it would work more smoothly if we used the webpack-middleware in the express server as opposed to starting our own webpack-dev-server.

@fbarl fbarl force-pushed the update-node-libraries-2016-11-17 branch from c27407f to 5ad11ef Compare November 22, 2016 15:20
@fbarl
Copy link
Contributor Author

fbarl commented Nov 22, 2016

@foot @davkal PTAL: I think I managed to make it work without webpack-middleware, but I had to weaken the cross-origin resource policy explicitly in the dev-mode, which I'm not sure is ok:

  new WebpackDevServer(webpack(config), {
    hot: true,
    noInfo: true,
    historyApiFallback: true,
    stats: 'errors-only',
    // We need the access from the express server for the hot-loader.
    // See: https://github.com/gaearon/react-hot-loader/issues/56
    headers: { "Access-Control-Allow-Origin": '*' },
  }).listen(4041, '0.0.0.0', function (err, result) {

Without this headers line, the problem would occur when hot loader would try to compile javascript with syntax error - it would first say it failed to access localhost:4041 from localhost:4042 and from there hot loading would basically get broken until you'd refresh the page.

@fbarl
Copy link
Contributor Author

fbarl commented Nov 22, 2016

@foot FYI If you think changing the cross-origin policy as I did is ok, I'd like to merge this with master and then try switching to webpack-middleware in a separate story as @davkal suggested.

@foot
Copy link
Contributor

foot commented Nov 23, 2016

Yeah the cross origin stuff looks fine.

  1. Breakpoints don't work after a hot-reload (not sure if fixable, and kind of work-around-able: ctrl-r).
  2. its very slow to hot-reload too (10s), though I think this was Fix source-maps/debugging in browser #1999
    • moving to cheap source maps brings it down to 3-4s for me, but you lose breakpoints completely.
    • Is it slow for you too?

It would be good to fix both the above but we could move them into new issues at this stage.

@fbarl fbarl merged commit 604661c into master Nov 23, 2016
@fbarl fbarl deleted the update-node-libraries-2016-11-17 branch January 17, 2017 10:02
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.

3 participants