8000 Proxy requests to webpack in development mode by smacker · Pull Request #145 · src-d/code-annotation · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@smacker
Copy link
Contributor
@smacker smacker commented Feb 20, 2018

Reasoning:

  • (a) I got tired of different versions of backend&frontend during development and 2 terminals
  • (b) I want to be able to run everything with only 1 command. Now it's make dev
  • (c) As Maximo correctly mentioned we are not building web service, we are building an application
  • (d) So in our case, web UI is just GUI like GTK or Qt. There are no advantages from ability to run it separately
  • (e) Env variables should come from BE anyway

Based on #143

Copy link
Contributor
@carlosms carlosms left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor
@carlosms carlosms left a comment

Choose a reason for hiding this comment

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

Docs need to be updated, right?

@smacker
Copy link
Contributor Author
smacker commented Feb 21, 2018

@carlosms yes. I just think we need to review/test/agree on it first. And if everything is ok, I'll update docs.

@bzz
Copy link
Contributor
bzz commented Feb 23, 2018

Trying it out and reviewing this takes some time. I'm sorry for delay @smacker, but most probably will be able to get back to this by Monday (latest).

@carlosms
Copy link
Contributor

Thinking more about this, I find it a bit strange to "pollute" the backend server with code to do proxy calls to the frontend development server.

Since this is only useful for frontend development, maybe we could rethink the solution to move the changes to the webpack server. package.json accepts proxy configuration even when create-react-app is used, maybe we could proxy all /api calls to the backend server this way.
We also have some conf. values injected by the backend into index.html. This could be solved with a proxy rule for index.html... Or the conf. could be served in a small json at /api/conf.

This is more a team conversation starter than a change request, I though it made sense to post it here for context even if we discuss this face to face.

@bzz
Copy link
Contributor
bzz commented Mar 14, 2018

Seems like the feedback was provided 2 weeks ago.
What would be the best way to move this forward and address it somehow @smacker ?

@smacker
8000 Copy link
Contributor Author
smacker commented Mar 14, 2018

@bzz I would like to see @dpordomingo feedback too.

smacker added 2 commits March 28, 2018 18:48
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Signed-off-by: Maxim Sukharev <maxim@sourced.tech>
Copy link
Contributor
@dpordomingo dpordomingo left a comment

Choose a reason for hiding this comment

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

Many thanks, @smacker for explaining clearly the motivations of this PR, and for taking your time to purpose this solution!!
I totally share your concerns about running the app for dev purposes (as easily as possible: with only one command, and using as less configuration as possible)

I rebased your branch against current master, tested it and it works.

After testing the tip of this branch and analyzing the proposed changes, I share the @carlosms concerns #145 (comment), plus these others:

  • running go server as a background job caused problems sometimes (server didn't stop when make dev ends, so next time you run it the 8080 port is already busy),
  • adding BROWSER="none" to the yarn start command, and using 8080 port, disables the hot reloading in development: one of the most useful features of the webpack server.
  • some of the motivations of this PR could not be fully satisfied:
    • using make dev does not avoid having problems with (a) "different versions of backend&frontend"; if it is changed the code of the repo while make dev is running, the frontend is automatically updated, but the backend is not,
    • I do not see how the purposed changes enforce (c) "the application point of view",
    • it's still needed to define the CAT_SERVER_URL so (e) "env variables should come from BE" is not enforced

Anyhow, I think this was a good starting point to solve the problems described by this PR, so I'll try to purpose an alternative based on the reasoning of this PR.

@smacker
Copy link
Contributor Author
smacker commented Apr 9, 2018

The project has been kind of freeze. So imo it doesn't worth to work on it anymore.

@smacker smacker closed this Apr 9, 2018
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.

4 participants

0