addon-dev: incremental updates to output #1855

patricklx · 2024-03-26T09:06:50Z

this now only updates changed files in dist. And deletes removed files in dist.
otherwise all files are deleted on each change, which makes HMR less useful

NullVoxPopuli

Logically, it seems good.
Do we have any tests for this behavior? I don't remember: 😅

mansona

so one thing that I would like to do before we actually move forward on this is that we verify that this Plugin works correctly with the bug scenario that #1448 was originally trying to fix. Ideally we would cover this in the v2 addon dev watch tests

patricklx · 2024-04-10T13:44:16Z

so one thing that I would like to do before we actually move forward on this is that we verify that this Plugin works correctly with the bug scenario that #1448 was originally trying to fix. Ideally we would cover this in the v2 addon dev watch tests

I think what this actually does now is more like a sync. When a file gets deleted in src, the corresponding generated file in dist also gets deleted.
there is the option runOnce https://github.com/vladshcherbin/rollup-plugin-delete?tab=readme-ov-file#runonce, which only deletes the files only once at start. Which is already better, but it leaves the deleted files during dev.

I think this also fixes the original issue. As long as files are copied as part of the bundle, by e.g emit, and not using manual copy. This will work correctly.

can we add a test? I could not reproduce the issue that caused the need for this

mansona · 2024-04-29T14:52:47Z

I think this also fixes the original issue

The problem I have with this PR is that I want more confidence that it fixes the original issue before we move forward with it. Myself and @NullVoxPopuli tried to recreate the issue on the Triage meeting and we couldn't get it to fail so we can't tell if this makes any difference 🙃

I would really like a failing test that shows the problem in a repeatable way before we try to fix it by altering the underlying implementation. If this is not possible (because maybe it's an OS level race condition 🤷) then at the very least we need to coordinate with @simonihmig because he said he would be looking into the original issue over the next while: embroider-build/addon-blueprint#32 (comment)

patricklx · 2024-04-29T16:24:29Z

I'm not sure how to add a test for this

patricklx · 2024-05-05T10:00:32Z

@mansona i improved this even more and added some tests

simonihmig

I tested this locally with a real project and yarn link. I found some minor issues, see inline comments. But the good part is that this seems to also fix some of the issues discussed in embroider-build/addon-blueprint#32 (comment).

The main issue I was hoping to get fixed by a revised clean plugin was the fact that an app watching the dist folder of an addon would often pick up changes as they were written but before the addon's rebuild has finished.
Leading to multiple app rebuilds, with the first ones often picking up interim build errors. Which would quickly go away, when the addon's rebuild has finished, but that's still annoying...

And testing this change here locally indeed seems to fix that issue! 🎉

packages/addon-dev/src/rollup-gjs-plugin.ts

tests/scenarios/v2-addon-dev-watch-test.ts

packages/addon-dev/src/rollup-incremental-plugin.ts

simonihmig · 2024-05-06T14:47:05Z

packages/addon-dev/src/rollup-incremental-plugin.ts

+        for (const file of files) {
+          if (!bundle[file]) {
+            generatedAssets.delete(file);
+            rmSync(join(options.dir!, file));


it seems watchChange provides us the information what files have changed or were deleted. Could we track these events and apply the syncing based on those, instead of the file diffing here?

Does that make any sense even? Not saying this is better or might not even work, just curious...

generated assets with emitfile do not pass through transform/watchchange. So it's not detectable

simonihmig · 2024-05-06T14:57:40Z

packages/addon-dev/src/rollup.ts

+  // rollup already supports incremental transforms of files,
+  // this extends it to the dist files
+  clean() {
+    return clean();


Do we consider this a breaking change? If not, this PR could target the stable branch, not?

If we consider this breaking, we could still release this on stable as a minor version, by exposing this as a separate plugin, and updating the blueprint to use the new one instead of addon.clean()! 🤔

packages/addon-dev/src/rollup-incremental-plugin.ts

tests/scenarios/scenarios.ts

tests/scenarios/v2-addon-dev-watch-test.ts

packages/addon-dev/src/rollup-hbs-plugin.ts

packages/addon-dev/src/rollup-incremental-plugin.ts

packages/addon-dev/src/rollup-gjs-plugin.ts

tests/scenarios/compat-stage2-test.ts

simonihmig · 2024-06-18T14:30:28Z

tests/scenarios/v2-addon-dev-test.ts

@@ -73,6 +73,8 @@ appScenarios
              exclude: ['**/-excluded/**/*'],
            }),

+            addon.clean(),


Is this change in order required? If so, then this is clearly a breaking change!

I think it is. I need to revalidate

Any news on this @patricklx?

With ~250 addons in our monorepo (not all v2 though), I'd rather not have to write a codemod to change the order! 😏

hi @simonihmig I reverted the change and the tests passed. (other unrelated failed)

hold that, i forgot one, lets see

There was a timeout in one test job. I restarted and it is all green now! 🎉

Thanks for getting back to this, I think this revert is helpful, as it shows we are not introducing breaking changes here!

tests/scenarios/v2-addon-dev-watch-test.ts

AskerFED · 2024-07-29T15:54:58Z

I have faced the ENOENT error for the addon.clean() which triggers file deletion event while rollup watch but it is not build stopping issue just error thrown and started build once dist was replaced. After @NullVoxPopuli suggestion to try this addon-dev branch now ENOENT error resolved. But the below warning is shown but is harmless I guess...

patricklx · 2024-08-07T13:15:06Z

packages/addon-dev/src/rollup-gjs-plugin.ts

@@ -14,11 +13,10 @@ export default function rollupGjsPlugin(
  return {
    name: PLUGIN_NAME,

-    load(id: string) {
+    transform(input: string, id: string) {


@ef4 I noticed that now the gjs plugin must come before the babel plugin in the rollup config. That would make this a breaking change

patricklx · 2024-08-07T13:15:41Z

packages/addon-dev/src/rollup-hbs-plugin.ts

@@ -12,6 +12,10 @@ export default function rollupHbsPlugin({
  return {
    name: 'rollup-hbs-plugin',
    async resolveId(source: string, importer: string | undefined, options) {
+      let hbsFilename = source.replace(/\.\w{1,3}$/, '') + '.hbs';
+      if (hbsFilename !== source) {
+        this.addWatchFile(hbsFilename);


@ef4 I had to add addWatchFile to all 3 methods to get this to work correctly on windows

ef4 · 2024-08-20T15:41:07Z

This is hard for me to follow. Can we take advantage of the cleanup that has happened in the vite hbs plugin to use the same patterns here?

patricklx · 2024-08-21T09:18:28Z

This is hard for me to follow. Can we take advantage of the cleanup that has happened in the vite hbs plugin to use the same patterns here?

I updated the hbs plugin to be similar to the one in vite hbs.
only a with a few changes. there is no generic extension search setup, it could be setup with @rollup/plugin-node-resolve
I added the extension search for .hbs extension in the hbs plugin.

also added the addWatchFile calls to get tests working

patricklx · 2024-08-21T11:11:52Z

packages/addon-dev/src/rollup-hbs-plugin.ts

+        }
+      }
+
+      let syntheticId = needsSyntheticComponentJS(source, resolution.id);


we do not have a packageCache here

patricklx · 2024-08-28T11:26:20Z

packages/addon-dev/src/rollup-hbs-plugin.ts

@@ -63,6 +63,7 @@ export default function rollupHbsPlugin({
      let syntheticId = needsSyntheticComponentJS(source, resolution.id);
      if (syntheticId) {
        this.addWatchFile(source);
+        this.addWatchFile(syntheticId);


this change did not work @ef4 .
I think transform does have its own specific cache.
https://rollupjs.org/plugin-development/#transform

patricklx · 2024-08-28T11:43:15Z

packages/vite/src/hbs.ts

-      let syntheticId = needsSyntheticComponentJS(source, resolution.id, resolverLoader.resolver.packageCache);
-      if (syntheticId) {
+      let syntheticId = needsSyntheticComponentJS(source, resolution.id);
+      if (syntheticId && isInComponents(resolution.id, resolverLoader.resolver.packageCache)) {


@ef4 is this okay like this?

no longer my PR :)

patricklx force-pushed the improve-addon-clean branch 2 times, most recently from aa83374 to 3883ada Compare March 26, 2024 09:26

NullVoxPopuli previously approved these changes Apr 10, 2024

View reviewed changes

mansona previously requested changes Apr 10, 2024

View reviewed changes

mansona added the waiting for contributor label Apr 29, 2024

patricklx force-pushed the improve-addon-clean branch 3 times, most recently from 94f3ccb to f472a50 Compare May 6, 2024 07:54

simonihmig requested changes May 6, 2024

View reviewed changes

simonihmig reviewed May 7, 2024

View reviewed changes

packages/addon-dev/src/rollup-incremental-plugin.ts Outdated Show resolved Hide resolved

patricklx force-pushed the improve-addon-clean branch from f130369 to 00ed454 Compare May 16, 2024 12:11

patricklx closed this May 16, 2024

patricklx reopened this May 16, 2024

patricklx commented Jun 6, 2024

View reviewed changes

tests/scenarios/scenarios.ts Outdated Show resolved Hide resolved

patricklx requested review from mansona and simonihmig June 7, 2024 18:01

patricklx force-pushed the improve-addon-clean branch from 9dd0b3a to a13adec Compare June 14, 2024 11:52

ef4 requested changes Jun 14, 2024

View reviewed changes

patricklx requested a review from ef4 June 17, 2024 10:48

patricklx force-pushed the improve-addon-clean branch 5 times, most recently from 0106e1e to 3b98560 Compare June 17, 2024 11:21

simonihmig reviewed Jun 18, 2024

View reviewed changes

patricklx force-pushed the improve-addon-clean branch from 89695de to 8f5af71 Compare July 11, 2024 15:54

patricklx closed this Jul 11, 2024

patricklx reopened this Jul 11, 2024

mansona removed their request for review July 29, 2024 14:45

patricklx requested review from ef4 and mansona August 7, 2024 07:44

patricklx commented Aug 7, 2024

View reviewed changes

patricklx force-pushed the improve-addon-clean branch from f7fc9e2 to b64444a Compare August 21, 2024 09:04

patricklx commented Aug 21, 2024

View reviewed changes

patricklx force-pushed the improve-addon-clean branch from 2bea12d to 1b9fcf1 Compare August 28, 2024 11:17

patricklx commented Aug 28, 2024

View reviewed changes

patricklx force-pushed the improve-addon-clean branch from d0d446c to b755ba2 Compare August 28, 2024 11:38

patricklx commented Aug 28, 2024

View reviewed changes

patricklx force-pushed the improve-addon-clean branch from b755ba2 to 1467bde Compare August 28, 2024 11:44

ef4 approved these changes Sep 3, 2024

View reviewed changes

patricklx force-pushed the improve-addon-clean branch from 41f2cd6 to 2cb99d7 Compare September 4, 2024 08:00

patricklx changed the title ~~improve addon-dev clean~~ addon-dev: incremental updates to output Sep 4, 2024

patricklx force-pushed the improve-addon-clean branch from 2cb99d7 to afb4998 Compare September 16, 2024 10:04

make rollup output incremental file updates

84c96d0

patricklx force-pushed the improve-addon-clean branch from afb4998 to 84c96d0 Compare September 18, 2024 08:36

ef4 merged commit 2e5e933 into embroider-build:main Sep 18, 2024
152 checks passed

github-actions bot mentioned this pull request Sep 18, 2024

Prepare Release #1873

Open

patricklx added a commit to patricklx/embroider that referenced this pull request Sep 20, 2024

backport embroider-build#1855 addon-dev: incremental updates to output

97878a6

patricklx added a commit to patricklx/embroider that referenced this pull request Sep 20, 2024

backport embroider-build#1855 addon-dev: incremental updates to output

47e1e03

mansona added the enhancement New feature or request label Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

addon-dev: incremental updates to output #1855

addon-dev: incremental updates to output #1855

patricklx commented Mar 26, 2024 •

edited

Loading

NullVoxPopuli left a comment

mansona left a comment

patricklx commented Apr 10, 2024 •

edited

Loading

mansona commented Apr 29, 2024

patricklx commented Apr 29, 2024

patricklx commented May 5, 2024

simonihmig left a comment

simonihmig May 6, 2024

patricklx May 6, 2024 •

edited

Loading

simonihmig May 6, 2024

simonihmig May 6, 2024

simonihmig Jun 18, 2024

patricklx Jun 18, 2024

simonihmig Jul 4, 2024

patricklx Jul 4, 2024

patricklx Jul 4, 2024

simonihmig Jul 9, 2024

AskerFED commented Jul 29, 2024

patricklx Aug 7, 2024

patricklx Aug 7, 2024

ef4 commented Aug 20, 2024

patricklx commented Aug 21, 2024 •

edited

Loading

patricklx Aug 21, 2024

patricklx Aug 28, 2024

patricklx Aug 28, 2024

addon-dev: incremental updates to output #1855

addon-dev: incremental updates to output #1855

Conversation

patricklx commented Mar 26, 2024 • edited Loading

NullVoxPopuli left a comment

Choose a reason for hiding this comment

mansona left a comment

Choose a reason for hiding this comment

patricklx commented Apr 10, 2024 • edited Loading

mansona commented Apr 29, 2024

patricklx commented Apr 29, 2024

patricklx commented May 5, 2024

simonihmig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricklx May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AskerFED commented Jul 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ef4 commented Aug 20, 2024

patricklx commented Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricklx commented Mar 26, 2024 •

edited

Loading

patricklx commented Apr 10, 2024 •

edited

Loading

patricklx May 6, 2024 •

edited

Loading

patricklx commented Aug 21, 2024 •

edited

Loading