embroider-build / embroider

Compiling Ember apps into spec-compliant, modern Javascript.
MIT License
329 stars 137 forks source link

improve addon-dev clean #1855

Open patricklx opened 1 month ago

patricklx commented 1 month ago

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

patricklx commented 1 month ago

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.

mansona commented 2 weeks ago

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: https://github.com/embroider-build/addon-blueprint/issues/32#issuecomment-2079582665

patricklx commented 2 weeks ago

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

patricklx commented 1 week ago

@mansona i improved this even more and added some tests