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

Frontend gulp tasks are complicated and doing too many things #2243

Closed
lfdebrux opened this issue Jun 4, 2021 · 9 comments
Closed

Frontend gulp tasks are complicated and doing too many things #2243

lfdebrux opened this issue Jun 4, 2021 · 9 comments

Comments

@lfdebrux
Copy link
Member

lfdebrux commented Jun 4, 2021

Cause

  • There are loads of sequences that build on top of each other.
  • There are tasks called both copy:assets and copy-assets.
  • Tasks are sometimes in files with different names
  • The scss:compile task is responsible for creating both the app CSS and the dist CSS, depending on a flag. The pipelines for these files are similar, but not identical, but completely different files are built depending on the flag.
  • The js:compile task is responsible for creating both the app JS and the dist JS, depending on a flag. The pipelines for these files are similar, but not identical, but completely different files are built depending on the flag.

Consequences

  • It's hard to make changes to our build pipeline
  • It's hard for contributors to understand how our build pipeline works
  • It's easy to run the wrong task because there are many similarly named tasks or ways of running things (e.g. gulp directly vs npm scripts)
  • We have a lot of dependencies, which means a greater attack surface, and more maintenance effort is required

Impact of debt

Medium, it works most of the time, but there is a high risk of breaking things when changes are needed.

Effort to pay down

High, we don't know enough about Gulp on the team, and we would want to be sure that any changes made don't break the build or lose functionality we need

Overall rating:

Medium

@lfdebrux lfdebrux added awaiting triage Needs triaging by team tech debt labels Jun 4, 2021
@lfdebrux
Copy link
Member Author

lfdebrux commented Jun 4, 2021

@lfdebrux
Copy link
Member Author

lfdebrux commented Jun 4, 2021

We'd probably need to pay this down before we can make changes to the build pipeline, i.e. if we want to change how we bundle JavaScript

@vanitabarrett
Copy link
Contributor

As discussed in the JavaScript effort/value sessions, one approach might be to tidy up our current Gulp tasks to make them easier to follow and so we have more confidence in making changes to them. However, this won't address the issues with the gulp dependencies we rely on being outdated.

We could also consider switching to an alternative to Gulp. This will require learning something new and moving all our existing tasks to that, and as a team we don't feel we know any other task runners any better than we do Gulp. There may be a steep learning curve, but there may be less maintenance overhead for us if this alternative is better maintained and supported.

@vanitabarrett
Copy link
Contributor

Just mentioning this issue here where we attempted to switch gulp-stylelint for stylelint but it was more complex than we originally though due to needing linting while running npm start which watches some of our Sass files. #2512

@domoscargin
Copy link
Contributor

domoscargin commented Jun 30, 2022

Posting here to say that a lot of the dependency problems stem from our use of oldie for IE8 styles compilation, which clashes with bumping most other css-related dependencies (the package itself is a bit of an oldie - latest release 7 years ago!). We might want to consider removing IE8 support in our browser support breaking release before working on this, since we could make these changes without a breaking release.

edit: Aha, just found #2469

@domoscargin
Copy link
Contributor

Going to add this comment here for future reference.

I think we should simplify our tasks and get rid of the destination option.

From what I can see:

  • The only task in common for build:package and build:dist is clean, and that does different things depending on which destination is set, so it probably makes sense just to make them two separate tasks.
  • The copy-to-destination file uses destination, but its copy-files task is only used in build:package
  • The compile-assets file uses destination, but again, that whole file is a bit of a mess and we could simplify it.
  • copy:assets is supposed to be used to copy to public, but it's actually only used for the dist build task
  • The only times we run a destination-reliant thing using the default of public are in the heroku and test scripts, so it feels like we'd probably be able to simplify that as well.

@colinrotherham
Copy link
Contributor

@lfdebrux @domoscargin Do you think we've done enough to reduce complexity and "too many things" to close this?

@lfdebrux
Copy link
Member Author

lfdebrux commented Nov 23, 2022

@colinrotherham I wasn't involved in writing the ticket (just copied it from an old Trello board #2243 (comment)), so possibly not the best person to ask, sorry!

@domoscargin
Copy link
Contributor

Given that the problems brought up on this issue have been dealt with, I'm going to close this ticket in favour of #2717 and update that issue to reflect any work remaining.

Our copyFiles task is still complicated and runs on largely abandoned plugins, but the situation is a lot better thant it was.

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

No branches or pull requests

5 participants