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

Clean up config files (and use new .config dir) #613

Merged
merged 14 commits into from
Apr 16, 2024

Conversation

Splines
Copy link
Member

@Splines Splines commented Apr 8, 2024

Pre-requisites

As versions of rubocop and eslint have changed, you will have to run:

bundle install
yarn install

For these new versions to also be available in the respective docker containers, you have to rebuild the images:

docker compose rm -s mampf
docker compose rm -s webpacker
docker compose build mampf webpacker
docker compose up -d

Maybe, as an alternative, one could also execute bundle install and yarn install inside the docker container but I haven't tried that.

Get rid of unused config files

We have a lot of config files at the root of this project. I removed some of them as they are not needed in my eyes:

  • .browserlistrc: used with browserslist as a config to share browsers and Node.js versions across different front-end tooling. We only had the word default in there. And I don't see which tools that we use at MaMpf would actually need this. So, let's get rid of it.
  • .travis.yml: Travis is a CI/CD solution. We use GitHub Actions right now and not Travis.
  • babel.config.js: This is a bit tricky one. The idea of babel is great: be able to use brand-new JS syntax but still support all browsers by compiling it down to a supported version as can be seen here. However,

Compiling in the browser has a fairly limited use case, so if you are working on a production site you should be precompiling your scripts server-side.

This is a warning you get when choosing "in the browser" here. See also the first sentence here. The only occurrence of the word "babel" I can find in our codebase is in lock files. That's why I'm pretty sure webpacker -- the tool we use to precompile all our assets -- does not even know about babel and the respective config file. Instead, we used a polyfill in the browser, which is dating back to August 2017 (!), see here.

Additionally, I'm pretty sure we don't leverage some very niche new JS features --at least none that modern browsers could not support. And ES6 is indeed supported in all modern browsers. Many of our files are written in coffeescript anyways. While I do think we should maybe support some browser up to maybe 1 to 2 years old, we should do this in precompilation steps already and not bundle a heavy polyfill on every browser request. If so, we should do this alongside #454 and in a manner such that we only really use polyfills where necessary and only components that we really need.

Deleting the babel polyfill from the _head.html.erb file will only make pages load faster. It might break the UI for users that are on 5 year old browser, but I don't think it's our job to support those.

  • .coffeelint.json: we want to get away from CoffeeScript and don't use this lint file right now in any tool. New JS files already make use of ESLint, so this should also be a motivation to move to JS to get proper linting ;)
  • .rspec: Removed here, will be added back in to the right location and with updated content in Add unit tests back to pipeline & improve VSCode integration #581
  • .erb-lint.yml: I left that one in as we have plans to use the respective shopify plugin as described in Ease compliance with code style guidelines: autoformatting, linting and VSCode settings #565
  • postcss.config.js: We make use of this file via the postcss-loader in webpacker, see here. However, I found it strange that while we do have the string "css-loader" in our codebase, we don't specify "postcss-loader". Still, somehow this file is loaded and trying to break it (e.g. remove a [) resulted in a ModuleBuildError during a test build in mampf-experimental. I couldn't find a way to move this configuration file to the .config/ folder, so let's leave it in the root for now and tackle this holistically within the realms of Webpacker has been retired / Upgrade frontend bundling #454.

Move config files to their own .config folder

Apart from that, for the moving of config files into their own .config/ folder, see this proposal.

Scope / Other files

  • Note that ignore-files such as .gitignore are exempt from this as their position relative to the root actually encodes a meaning, e.g. one can have gitignores just for subdirectories.
  • Other files such as entrypoint.sh or initialize.sh might be moved to a better place (but not .config/) in a subsequent PR.
  • Gemfile and package.json as well as the respective lock files stay in the root.
  • License and markdown files have to be in the root to be correctly recognized by GitHub, e.g. to see a proper link to the contribution guide.
  • I stayed away from config.ru and Rakefile so far as I fear breaking the build. Maybe, they can be tackled in a possible future PR.
  • mampf-gui-transparent.png will be deleted in a subsequent PR that will aim at improving the Readme.

TODO

  • Click through MaMpf locally to ensure, the website and some basic functionality is working as expected even without the 7 years old babel polyfill.
  • Check that mampf-experimental is running through without any build errors.

For reviewers

Apart from all other mentioned "things", please consider these points in particular:

  • Make sure the ESLint VSCode plugin is still properly linting our .js and .js.erb files automatically on save and according to our rules.
  • Make sure ruby files are correctly linted according to rubocop (via the VSCode plugin automatically on save). Additionally, running bundle exec rubocop should yield exactly 15 offenses (that will be autofixed in a subsequent PR).
  • Optional: Maybe try out other browsers and click through MaMpf to ensure that at least basic functionality is working as expected even without the 7 years old babel polyfill.
  • Other things: <specify here by editing this comment. Note, reviewers can also edit the comment.>

During the build, note that the following warnings have already been issues even before this PR:

warning "@stylistic/eslint-plugin > @stylistic/eslint-plugin-plus > @typescript-eslint/[email protected]" has incorrect peer dependency "eslint@^7.0.0 || ^8.0.0".
warning "@stylistic/eslint-plugin > @stylistic/eslint-plugin-plus > @typescript-eslint/utils > @typescript-eslint/typescript-estree > [email protected]" has unmet peer dependency "typescript@>=4.2.0".

@Splines Splines added the dependencies Pull requests that update a dependency file label Apr 8, 2024
@Splines Splines self-assigned this Apr 8, 2024
@Splines Splines marked this pull request as ready for review April 8, 2024 19:28
@Splines Splines marked this pull request as draft April 9, 2024 16:45
@Splines Splines changed the title Remove unused config files Clean up config files (and use new .config dir) Apr 9, 2024
@Splines Splines marked this pull request as ready for review April 9, 2024 20:09
@Splines Splines merged commit 098da70 into dev Apr 16, 2024
3 checks passed
@Splines Splines deleted the config/clean-up-config-files branch April 16, 2024 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants