v2 addon watching not working with BROCCOLI_ENABLED_MEMOIZE

[simonihmig]

When this flag is enabled, any changes in a watched addon do trigger a rebuild, with the changed file being mentioned in the terminal output, so watching itself is working, but the actual changes don't get picked up.

This is happening both in classic builds with eai and watchDependencies and with Embroider/Webpack and broccoli-side-watch set up.

Here is a reproduction, see it's Readme for steps to reproduce.

My current working theory is as follows:
What both watchDependencies and broccoli-side-watch basically do is wrapping the main app tree with a broccoli-merge-trees that includes WatchedDir nodes for the addon dist output, something like this (example for eai, but I think it would be similar for Embroider):

.
└── MergeTree
    ├── App.toTree()
    │   └── eai webpack tree
    │       └── webpack subsystem
    ├── NoOp
    │   └── WatchedDir (addonA/dist)
    ├── NoOp
    │   └── WatchedDir (addonB/dist)
    └── ...

So when some addon files change, the WatchedDir will cause broccoli to get notified to do a rebuild. Without the memoization flag, it will do a full build again, including the whole app tree and webpack stuff, that will then rebuild the code including the changed file. But with the flag enabled, the app tree node will think that nothing has changed (because none of the addon files are actually inputNodes anywhere in that tree, so broccoli does not know about them being involved, only webpack does), so will basically get short-circuited and not do a rebuild nut return the previous output.

Again, just my theory, does that make sense?

What puzzles me is the fact that Embroider has this flag enabled by default. So how was this working before under Embroider? Was it even?

Note that I worked on a couple of addon watching related PRs, but they focused on narrowing down the watched folders to not include e.g. src. And given that the changed files do trigger a rebuild, I don't think these can have caused that. But in case it helps investigating the issue, I am listing these here:

• Add better broccoli-side-watch package #2141
• [addon-shim]: Narrowed down broccoli trees for optimized file watching #1901
• Optimize watched directories ember-auto-import#623

Note that we only started to observe this now because we had some v1 addon that was doing some broccoli trickery that included having v2 dist files as its own broccoli inputs, so in that case that broccoli tree would accidentally cause the proper rebuilds I think. That would explain why we didn't came across this earlier...

[patricklx]

@simonihmig the way you using sideWatch seems to be wrong. I compared it with the solution in #1892 (comment)

In you repo, When it applies the watched dirs, its already to late. the packager already received the trees from app instance.
the correct setup is

module.exports = function (defaults) {
  let app = new EmberApp(defaults, {
    trees: {
      app: sideWatch('app', {
        watching: ['watch-test'],
      }),
    },
  });

  return Compat.compatBuild(app, Webpack);
};

[simonihmig]

You are right @patricklx , thanks a bunch!

This also explains why I came up with the diagram above, I think it comes close to what I was getting with the previous wrong setup, but with your suggested changes, the watched dirs become inputs to the compatBuild, so rebuilding works correctly.

However, the issue remains for classic builds. Not sure why though, as the WebpackBundler here does seem to receive the watched dirs as inputs as well, but my repo still reproduces this not working with the memoization flag turned on! 🤔

We can get around this though by not using watchedDependencies but the same side-watch approach even in classic apps. I don't know if there is still enough appetite to get this fixed in eai?

Closing this one, as the Embroider part seems to be ok. Can create another issue in the eai repo if enough people care? /cc @ef4

[patricklx]

I do not see the issue with the classic build in your repo

[simonihmig]

When you run the classic-app with BROCCOLI_ENABLED_MEMOIZE=true pnpm start as described in the Readme, does it pick up changes for you? It doesn't for me!

[patricklx]

you right, i didn't try with BROCCOLI_ENABLED_MEMOIZE=true. i took a look... and fix

[ef4]

I don't know if there is still enough appetite to get this fixed in eai?

The fix there involves using webpack's own file watching, not just broccoli's. We're pretty sure it would also improve rebuild times. It would be a good change, if somebody wants to work on it we could pair on getting started.

[patricklx]

I don't know if there is still enough appetite to get this fixed in eai?

The fix there involves using webpack's own file watching, not just broccoli's. We're pretty sure it would also improve rebuild times. It would be a good change, if somebody wants to work on it we could pair on getting started.

Already done
embroider-build/ember-auto-import#652

At least on broccoli side

[simonihmig]

Already done

Is it? Ci is pretty red! :)

[patricklx]

Already done

Is it? Ci is pretty red! :)

still babel/traverse issues...hopefully they merge the revert soon.
oh, just now: babel/babel@f33704a

[ef4]

That would enable using sideWatch as a workaround, but it's not the same thing I was describing.

[simonihmig]

That would enable using sideWatch as a workaround

Are you saying that the same side-watch based approach would not work with eai? Because it seems to work fine for me!?

[ef4]

Are you saying that the same side-watch based approach would not work with eai? Because it seems to work fine for me!?

No, I was apparently misunderstanding that this bug report claimed that doesn't work.

[simonihmig]

No, it's just the watchDependencies feature of eai itself that does not work with memoization. Using side-watch for both Embroider (stable) and eai seems to work fine. (given the proper setup, which I had messed up before)