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

Watcher semantics are incorrect for use with BROCCOLI_ENABLED_MEMOIZE #9020

Open
ef4 opened this issue Jan 29, 2020 · 6 comments
Open

Watcher semantics are incorrect for use with BROCCOLI_ENABLED_MEMOIZE #9020

ef4 opened this issue Jan 29, 2020 · 6 comments
Labels

Comments

@ef4
Copy link
Contributor

ef4 commented Jan 29, 2020

Places like this:

stylesTree = new UnwatchedDir(this._stylesPath);

that use UnwatchedDir are assuming that complete rebuilds will be triggered by the parent path's watcher. But that violates the assumptions that make BROCCOLI_ENABLED_MEMOIZE work. It's supposed to know which trees need to rebuild based on which files the watcher mentions. An UnwatchedTree will never rebuild, and then its downstream trees will also not rebuild.

However, it turns out this bug is cancelled out by other bugs, and apps do still get rebuilding of their styles. But it happens by accident, because

  1. the app styles tree gets merged with many other trees and then and re-funneled before getting processed, which (incorrectly) causes it to depend on additional trees that are WatchedDirs and not UnwatchedDirs. This contagion causes later stages to run even though app.trees.styles itself never updates.
  2. We get lucky with the broccoli directory symlinking. So even though the app.trees.styles tree never updates, the changes propagate forward via the symlinked directory.

I came upon this behavior because of the way Embroider consumes the app's styles. It depends on app.trees.styles, and that works with BROCCOLI_ENABLED_MEMOIZE off because even though that tree never updates, trees downstream from it would always run and would follow the symlinking to the updated files. But with BROCCOLI_ENABLED_MEMOIZE enabled, app CSS fails to rebuild under embroider because we really need the tree to update correctly.

@rwjblue
Copy link
Member

rwjblue commented Feb 11, 2020

FWIW, the reason this is using UnwatchedDir is that app/styles is within the app tree. The fix here would be to confirm that we are using the nested dir, and use a funnel off of the trees.app instead of making a second UnwatchedDir. This way the downstream tree would properly invalidate/rerun upon change, but still avoid indicating two files changed (one for styles/foo.scss and another for foo.scss which is what was happening waaaaaay back when).

@ef4
Copy link
Contributor Author

ef4 commented Feb 11, 2020

That makes sense. So I think in detail it would be:

  1. If the user provided their own broccoli tree as app.trees.styles, we just use that and there's nothing else to do.
  2. If the user provided a string (or nothing, which we default to the string app/styles) for app.trees.styles, we need to know whether it's nested inside their app tree.
    1. If the user provided a string (or nothing, which we default to the string app) for their app.trees.app, we can compare that with their app.trees.styles string to see if we have a subdir
      1. If its a subdir, we can funnel off the app tree.
      2. Otherwise we make a new watched root.
    2. if the user provided a broccoli tree as app.trees.app, we are out of luck and can't tell whether the styles are nested inside it. We could make a watched root and just accept the possibility of double watching.

@locks locks added the watcher label Feb 10, 2022
@angelayanpan
Copy link

was this resolved? @ef4 did anything change from 2020?

@ef4
Copy link
Contributor Author

ef4 commented Apr 20, 2022

I don't think anything is changed, embroider still ships with a workaround for this. But if you are seeing a problem in your apps, please report it because the workaround in embroider is supposed to make this not matter.

@angelayanpan
Copy link

in classic ember build, we are not able to use the BROCCOLI_ENABLED_MEMOIZE flag because the re-build doesn't update scss changes.

@ef4
Copy link
Contributor Author

ef4 commented Apr 20, 2022

Ah, so when I reported this it wasn't actually causing any problems in classic apps. So maybe something has changed.

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

No branches or pull requests

4 participants