improve addon-dev clean #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! 🎉

simonihmig · 2024-05-06T14:28:02Z

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) {


This change and the one on the hbs plugin are unrelated to the actual scope of the PR, right?

It's required to have hbs pass through transform to have thrm incremental.

simonihmig · 2024-05-06T14:29:44Z

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

-            addon.publicEntrypoints(['components/**/*.js']),
-            addon.appReexports(['components/**/*.js']),
+            addon.publicEntrypoints(['components/**/*.{gts,js}']),
+            addon.appReexports(['components/**/*.{gts,js}']),


Are these changes needed? IIRC, the inputs to these plugin were meant to relate to the final output, which is always .js.

Ah, maybe it wasn't needed

simonihmig · 2024-05-06T14:36:43Z

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

+    },
+    generateBundle(options, bundle) {
+      if (existsSync(options.dir!)) {
+        const files = walkSync(options.dir!, {


any concerns about this file walking being potentially too expensive to do on every rebuild?

It's required to delete files that are not part of the bundle. We could also do this with diffing between builds and only do a walkSync at start.

I improved this ti only walk once

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

this only cleans up deleted files. otherwise all files are deleted everytime, which makes HMR less useful

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 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 and others added 10 commits May 16, 2024 14:08

add own clean plugin

4847379

this only cleans up deleted files. otherwise all files are deleted everytime, which makes HMR less useful

add test

baf8d19

gjs caching

48ee08e

assets cache

43a1912

hbs cache

b1b2000

test assets

47a76f2

support colocation

fedc3d2

rename to incremental

6621d6b

fix asset check

12e5793

improve hbs transform

00ed454

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 added 2 commits May 16, 2024 15:20

fix windows path

3112322

fix test

002c9e6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve addon-dev clean #1855

improve addon-dev clean #1855

patricklx commented Mar 26, 2024 •

edited

NullVoxPopuli left a comment

mansona left a comment

patricklx commented Apr 10, 2024 •

edited

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

simonihmig May 6, 2024

patricklx May 6, 2024

simonihmig May 6, 2024

patricklx May 6, 2024 •

edited

patricklx May 16, 2024

simonihmig May 6, 2024

patricklx May 6, 2024 •

edited

simonihmig May 6, 2024

simonihmig May 6, 2024

improve addon-dev clean #1855

Are you sure you want to change the base?

improve addon-dev clean #1855

Conversation

patricklx commented Mar 26, 2024 • edited

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

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

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

patricklx May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patricklx May 6, 2024 • edited

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

patricklx commented Apr 10, 2024 •

edited

patricklx May 6, 2024 •

edited

patricklx May 6, 2024 •

edited