Skip to content

feat(compilation): force asset compilation on flush instead of deletion#3582

Closed
luceos wants to merge 1 commit intomainfrom
dk/recompile-frontend-assets
Closed

feat(compilation): force asset compilation on flush instead of deletion#3582
luceos wants to merge 1 commit intomainfrom
dk/recompile-frontend-assets

Conversation

@luceos
Copy link
Copy Markdown
Contributor

@luceos luceos commented Aug 5, 2022

Changes proposed in this pull request:

Instead of deleting compiler generated assets we will now rebuild them on cache clear.

The reason we're doing this is this scenario mainly:

  • cronjob or queue worker is running
  • admin clears cache and doesn't visit the site thereafter
  • no assets are generated
  • cronjob or queue worker is processing a task that needs one of the assets, eg sending notification email with lang keys or style

Reviewers should focus on:

  • Are we sure we want to commit thereafter
  • Are other changes required that I've overlooked

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@SychO9
Copy link
Copy Markdown
Contributor

SychO9 commented Aug 17, 2022

Technically, isn't the only problem with locale being unavailable from non-web processes? if so I think we should only commit that rather than everything else and add a comment mentionning why.

@davwheat
Copy link
Copy Markdown
Contributor

It is, but I think changing this would make the class do what it's expected to do compared to what it does now.

@SychO9
Copy link
Copy Markdown
Contributor

SychO9 commented Aug 17, 2022

That's just a naming problem though.

The class is used a lot throughout the code-base for many post-operations (ext enable, disable, settings save, cache clearing ...etc) in addition to the fact that certain extensions call this as well (package manager). There was probably a good reason behind why it flushes assets to delay recompilation to a later time/process.

imo, the main problem here is that the console needs assets to be compiled but doesn't trigger compilation itself. Then the safer solution is to adapt the console to trigger asset compilation.

@luceos
Copy link
Copy Markdown
Contributor Author

luceos commented Aug 18, 2022

The fear to change something that works that it might break something unforeseen.

I think it's broken as is and I think we need to modify this logic to do asset compilation as soon as possible. Or we need to move compilation away from the http stack to the application bootstrapping or we need to make compilation smarter about it's contextual use (compile languages when it's needed versus compile js/css only for web).

@SychO9 SychO9 added this to the 1.6 milestone Sep 6, 2022
@SychO9 SychO9 modified the milestones: 1.6, 1.7 Nov 15, 2022
@SychO9
Copy link
Copy Markdown
Contributor

SychO9 commented Nov 15, 2022

Created an issue for it (#3680), I still believe we should fix the specific issue at hand rather than make radical changes to the behavior. We can revisit refactoring the behavior itself when we have a clear plan of why and how. But the bug IMO should be tackled with minimal & changes consistent with the current codebase.

@luceos
Copy link
Copy Markdown
Contributor Author

luceos commented Nov 23, 2022

I accept your wisdom in this @SychO9.

@luceos luceos closed this Nov 23, 2022
@SychO9 SychO9 deleted the dk/recompile-frontend-assets branch January 5, 2023 12:29
@SychO9 SychO9 removed this from the 1.7 milestone Jan 14, 2023
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.

3 participants