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

Switch to a different bundler #376

Closed
ddohler opened this issue Nov 9, 2021 · 5 comments · Fixed by #457
Closed

Switch to a different bundler #376

ddohler opened this issue Nov 9, 2021 · 5 comments · Fixed by #457
Assignees
Milestone

Comments

@ddohler
Copy link
Contributor

ddohler commented Nov 9, 2021

Currently the project uses Mastarm, but that project is a custom Conveyal tool that is no longer maintained, and we've had to fork it to fix bugs. Switching to a more mainstream tool such Webpack would improve maintainability of the project.

Part of this should include adjusting how environment variables are handled for the frontend and pulling them in from S3 via scripts/bootstrap. See this discussion for details.

We should also move taui into the src directory.

@aaronxsu
Copy link
Member

Hey @ddohler , I made some notes after some high-level investigation, and would like to ask you to review if the proposed changes make sense before proceeding.

cc @rachelekm and @ktohalloran , please also take a look and feel free to join the discussion!

If there is any infra related changes, I will make notes of them along the way and let Bryan know after he comes back from vacation.

Overview

I did some light investigation today trying to understand better the scope of the issue- what the current status is, what options we have to decouple and replace, and what related scripts for local dev and CI/CD will need updates. I laid out my understanding of these aspects in the following sections, and would like to invite folks to join this discussion, since this is an 8-point card and it potentially has some downstream (positive) impact for development and deployment.

What is the current status?

In order to remove mastarm and replace it with more mainstream alternatives, we need to understand what the current status is. The frontend of this app uses mastarm for a handful of tasks which seem to have extended what I think a conventional bundler would do (to my understanding). mastarm is used for the following tasks, as specified in the task runner scripts in package.json.

  • transpile and build the app as static assets
  • transpile and run the app locally with a proxy config
  • lint JS code
  • lint CSS code
  • handle deployment of the app to AWS

In addition to the above, mastarm also consumes some yaml files under /configurations for different types of tasks:

  • env.yml to store environment variables for the app runtime
  • store.yml to store initial values for some fields in the redux store
  • settings.yml- this is something I am not entirely sure, but it looks like it is mainly for deployment purpose

What to decouple and replace?

mastarm handles of lot of tasks through CLI commands and arguments, which are used as task runner scripts in the app frontend. Additionally, it also has some dependencies that are critical as devDependencies for this app frontend. We need to decouple them and replace with the following mainstream options:

  • babel and relavent presets/plugins for transpiling
  • webpack and webpack-cli for bundling, and we need some configs
  • webpack-dev-server for running and serving a local app for development with a proxy for local dev
  • dotenv-webpack to pick up the environment variables from a .env file (to be derived from env.yml)
  • eslint and its related plugins and configs for linting JS
  • prettier for code formating
  • flow-bin and flow-runtime

The above may not be an exhaustive list, as some non-obvious dependencies may be required after removing mastarm.

Plus, the initial values of some fields in the redux store may need to be included from store.yml. But I am not sure if they should be environment-specific or just be hard-coded. Any suggestions?

For configurations in settings.yml- it is unclear to me how relevant these are and if we should add them to tfvars so that they will be picked up by terraform. Any pointers on this one would be great.

What other changes do we need?

  1. We will also update the directory structure of this app, so that backend and frontend will both be under src
  2. As a result, docker-compose's taui service needs new paths for volume mounting
  3. ./scripts/bootstrap may need updates if we want to pull .env from S3 for those values in env.yml
  4. ./scripts/cibuild needs updates so that app frontend is built by this new bundler before building django
  5. docker-compose's django service needs to have the frontend static assets directory mounted as a volume so that django can find the frontend app

@ddohler
Copy link
Contributor Author

ddohler commented Apr 21, 2022

Thanks for writing this up, @aaronxsu ! This looks like a pretty comprehensive list. I've got a few comments about some of the points you mentioned:

dotenv-webpack to pick up the environment variables from a .env file (to be derived from env.yml)

I'm not sure whether this will be necessary -- if we use Webpack from inside docker-compose, then I think docker-compose should inject the environment variables from the .env file into the Webpack container, and then Webpack can grab them directly from the environment rather than parsing .env itself. But I'm not 100% certain about this, so this makes sense to add if this doesn't work the way I'm imagining.

eslint and its related plugins and configs for linting JS
prettier for code formating

One thing to note is that Mastarm actually uses https://github.com/prettier/prettier-eslint for formatting, which formats files by running prettier followed by eslint --fix (because there are things that they disagree about). However, I don't think we need to keep using exactly this tool--we can move formatting to be more in line with our approach on other Javascript projects. I think it's fine if this task reformats the code as long as the commits are broken out from everything else.

Plus, the initial values of some fields in the redux store may need to be included from store.yml. But I am not sure if they should be environment-specific or just be hard-coded. Any suggestions?

I don't believe that any of the parameters in store.yml are parameterized per-environment, so I think they can just be hard-coded. I checked the echo-locator-*-config-us-east-1 buckets on S3 and they only have env.yml, not store.yml, which supports that belief.

For configurations in settings.yml- it is unclear to me how relevant these are and if we should add them to tfvars so that they will be picked up by terraform. Any pointers on this one would be great.

The fields in settings.yml are documented here: https://github.com/conveyal/mastarm#settingsyml . However, I don't think we need to preserve the existing deployment process. It would be good to confirm with @BryanQuigley , but I don't think there's any overlap between the Django deployment process and the existing deployment process, which is based on S3 + Cloudfront. We should focus on getting the Django deployment working so that static files can be served via Django.

I hope that all is helpful!

@aaronxsu
Copy link
Member

Thanks @ddohler for the detailed comments and answers. These are very helpful!

@rachelekm
Copy link
Contributor

Thanks for writing this up and including me in the discussion, this looks great and is helpful context!

@ktohalloran
Copy link
Contributor

ktohalloran commented Apr 22, 2022

I second @rachelekm --thanks for including us! I look forward to saying goodbye to mastarm 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants