Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

web: add web-app server for development and production builds #20126

Merged
merged 21 commits into from
May 7, 2021

Conversation

valerybugakov
Copy link
Member

@valerybugakov valerybugakov commented Apr 19, 2021

Independent web app server

This PR introduces an independent server for the web application which can use any deployed API instance as a backend.

Context

While working on the CSS modules configuration, I needed a way to iterate quickly on the webpack.config.js and test changes in the production environment: run yarn build-web and check how CSS modules are loaded in the application. There was no easy/quick way to do it. @umpox found out that it's possible to change a couple of variables in the start.sh script to serve the production build. But this solution was quite slow because of the backend services involved.

To solve this, I've configured a minimum viable server to serve web app production artifacts without our backend services. I've added HtmlWebpackPlugin to our webpack.config.js to produce index.html and configured Webpack dev server proxy to point the server to the deployed API instance. It allowed testing changes in production build without backend services quickly. This PR is the enhanced version of this setup.

How it works

  1. HtmlWebpackPlugin is added to the Webpack config. It can be enabled by the env variable WEBPACK_SERVE_INDEX=true. This plugin outputs index.html to the ui/assets.
  2. Express server is used for a production build. Webpack-dev-server in the development environment.
  3. Server proxies API requests to SOURCEGRAPH_API_URL provided in the .env file using the http-proxy-middleware.
  4. To avoid the CSRF token is invalid API error CSRF token is retrieved from the SOURCEGRAPH_API_URL before the server starts. Then this value is used for every subsequent request to the API.
  5. To enable authentication via the sgs cookie, all cookies from the API requests are propagated to the local client.

Known issues

The issues listed below can be addressed in separate PRs. They are non-blocking for the functionality this PR introduces and doesn't affect existing workflows. We can iterate on each individual issue once we have the foundation in place.

  1. HTTPS support is not in place yet, because of (HTTPS broken on Chromium browsers webpack/webpack-dev-server#2313). It needs more investigation.
  2. A static mock in combination with a local site-config.json is used instead of the JS context returned from the SOURCEGRAPH_API_URL.
  3. Authentication providers don't work. This is probably related to the social login links, which are served along with JS context.
  4. A couple of variables are duplicated from gulpfile.js. They should be shared with it after migration to Typescript.
  5. index.html template is different from what we have in cmd/frontend/internal/app/ui/app.html. We should reuse it to be consistent with the default production setup.

Changes

  1. Two commands are added to the client/web/package.json: yarn serve:dev and yarn serve:prod.
  2. dev directory is added to the client/web package. It contains the server configuration used by the commands above.
  3. .env.example is added to the client/web to showcase the example env configuration.
  4. require('ts-node').register() is added to the gulpfile.js to allow TS usage for html-webpack-plugin configuration.

Updates

  1. Added four commands to integrate web server with sg CLI tool:
  • sg run web-standalone
  • sg run enterprise-web-standalone
  • sg run web-standalone-prod
  • sg run enterprise-web-standalone-prod
  1. Removed .env.example and dotenv package because env variables are now defined in sg.config.yaml.
  2. Added global alert about API proxy usage.

Screenshot 2021-05-05 at 14 15 12

@valerybugakov valerybugakov added ops & tools & dev frontend-platform Issues related to our frontend platform, owned collectively by our frontend crew. labels Apr 19, 2021
@valerybugakov valerybugakov self-assigned this Apr 19, 2021
@valerybugakov valerybugakov marked this pull request as ready for review April 28, 2021 14:19
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

Nice work 🎊

I would be worried a bit about people messing with sourcegraph.com, but maybe that's not a reasonable fear :) I think we should not make it serve on sourcegraph.test:3443, though because it's too easy to think it's a safe dev environment.

  • A static mock is used instead of the JS context returned from the SOURCEGRAPH_API_URL.

I think that should be implemented soon. The flags set change a lot about the app behavior and it is also dynamic in the backend so if I change a config, the jscontext would change if it was a real deployed app.

  • index.html template is different from what we have in cmd/frontend/internal/app/ui/app.html. We should reuse it to be consistent with the default production setup.

Definitely agree. Also it's so easy to forget about this file when someone updates the app.html file.

client/web/dev/utils/get-site-config.ts Outdated Show resolved Hide resolved
client/web/dev/utils/get-site-config.ts Show resolved Hide resolved
client/web/dev/utils/get-site-config.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,65 @@
import 'dotenv/config'
Copy link
Member

Choose a reason for hiding this comment

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

Now that we already have quite a few packages, I think this should be it's own package under client/dev-server or whatever. That way, this package will still be only code required in the webapp, and it's not as easy to accidentally import webpack and such into the final web bundle (accidentally imported from this file, for example). Also the dev server needs commonjs as a target, and the webapp can be esnext, so maybe another indicator that a proper package with it's own tsconfig.json file might make sense here.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the right call. I thought about doing it in this PR. Still, apart from this change, we need to extract shared Webpack configuration into a standalone package to avoid adding loaders to three different configs: Web, Browser, and Storybook. I believe this step should be done before extracting client/dev-server because it might result in duplicated work and circular dependencies between web and dev-server packages. First, move configs to a shared package. Then extract dev-server and use shared configs.

client/web/README.md Outdated Show resolved Hide resolved
client/web/dev/server/production.server.ts Outdated Show resolved Hide resolved
client/web/dev/webpack/get-html-webpack-plugins.ts Outdated Show resolved Hide resolved
client/web/dev/server/development.server.ts Show resolved Hide resolved
client/web/gulpfile.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
client/web/dev/server/development.server.ts Show resolved Hide resolved
@felixfbecker
Copy link
Contributor

I'm really excited to enable web dev without needing any backend services.

@felixfbecker
Copy link
Contributor

Something we may want to consider is injecting some kind of banner somewhere "THIS USES PRODUCTION DATA FROM sourcegraph.com" because it may be easy to forget if you have multiple or old tabs open given this runs on localhost. I feel like it may otherwise only be a matter of time until someone accidentally misconfigures an external service on dotcom or something

@valerybugakov valerybugakov added the dx Issues and PRs related to developer experience concerns label Apr 29, 2021
Copy link
Contributor

@umpox umpox left a comment

Choose a reason for hiding this comment

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

After following the READM

client/web/README.md Outdated Show resolved Hide resolved
client/web/README.md Outdated Show resolved Hide resolved
client/web/.env.example Outdated Show resolved Hide resolved
client/web/README.md Outdated Show resolved Hide resolved
client/web/README.md Outdated Show resolved Hide resolved
@felixfbecker felixfbecker requested a review from a team April 30, 2021 14:27
@vovakulikov
Copy link
Contributor

vovakulikov commented May 3, 2021

Nice work. I don't think this is a blocker for this, but just the thing which we should notice -
for some reason we have strange visual behaviour in safari with this approach. Probably some styles for Monaco editor are missing.

image

sg.config.yaml Outdated Show resolved Hide resolved
@valerybugakov
Copy link
Member Author

Updates

  1. Added four commands to integrate web server with sg CLI tool:
  • sg run web-standalone
  • sg run enterprise-web-standalone
  • sg run web-standalone-prod
  • sg run enterprise-web-standalone-prod
  1. Removed .env.example and dotenv package because env variables are now defined in sg.config.yaml.
  2. Added global alert about API proxy usage.

Screenshot 2021-05-05 at 14 15 12

Copy link
Member

@andreeleuterio andreeleuterio left a comment

Choose a reason for hiding this comment

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

LGTM! What matters more for security here (please add anything I didn't think of!):

  • Makes devs lives easier ✨
  • Uses k8s.sgdev as default
  • Alerts users of which instance they're connected to
  • No HTTPS yet because of upstream bug

@valerybugakov valerybugakov merged commit 8469a10 into main May 7, 2021
@valerybugakov valerybugakov deleted the vb/web-server branch May 7, 2021 11:01
@valerybugakov valerybugakov mentioned this pull request May 7, 2021
28 tasks
vovakulikov pushed a commit that referenced this pull request May 8, 2021
This PR introduces an independent server for the web application, which can use any deployed API instance as a backend.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dx Issues and PRs related to developer experience concerns frontend-platform Issues related to our frontend platform, owned collectively by our frontend crew. ops & tools & dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants