embroider-build / addon-blueprint

Blueprint for v2-formatted Ember addons
MIT License
29 stars 24 forks source link

Rollup clean plugin causes noisy build errors #32

Open ef4 opened 2 years ago

ef4 commented 2 years ago

I think that the clean we have in the rollup config causes transient-but-noisy build errors in the test-app. Because it starts the build by removing everything in dist, the test-app can rebuild and find things missing and get build and/or type errors.

This seems better if you remove clean. The downside is that there is no automatic removal of build artifacts if you really do delete something from your source.

simonihmig commented 2 years ago

Yeah, I have seen these build errors come up as well, but didn't have the time yet to investigate. Thanks for the hint!

Both alternatives are not really ideal though. Is there any way we can do this "properly". Like maybe do the build in a temporary folder, and when done move it to dist as basically an atomic operation, so Ember-CLI's watcher will not see any transient state? For lack of a better way to signal Ember-CLI that "we are done, you can take over now". Just thinking aloud here...

ef4 commented 2 years ago

Yeah, an atomic update would probably be best. Maybe there is a rollup plugin that does that.

simonihmig commented 2 years ago

Fix: https://github.com/embroider-build/embroider/pull/1229

simonihmig commented 2 years ago

Should be fixed by the PR above, so closing this. Should this come up again, then please reopen!

simonihmig commented 1 year ago

I encountered some related problems again:

So I guess we should consider again having an atomic update eventually. Therefore re-opening this!

mansona commented 1 year ago

Hey folks 👋 I've come across similar problems that are related to the fact that we're not doing an atomic build. Essentially if you're running npm start in the v2 addon while also running npm start in a test app (or any classic app consuming the v2 addon) you will get multiple rebuilds in the app. Once when you update the source file, and once again when your rollup job is finished updating the dist.

I did some investigation and it doesn't seem like implementing an atomic update in Rollup is much of a priority 🤔 I couldn't find any plugins that do this for you. The closest I could find was https://www.npmjs.com/package/@mprt/rollup-plugin-incremental but I'm getting the feeling that this doesn't quite do what we are hoping for

NullVoxPopuli commented 1 year ago

An idea I had to help with the issue (but not solve it), only helps out auto-import users, where we change the behavior of autoImport.watchDependencies to look at a package's package.json#files list, and only watch those changes (falling back to current behavior of files isn't specified). That would ignore src file changes (unless src is in files). We'd want to keep watching package.json changes so we wouldn't be able to eliminate all unneeded rebuilds.

simonihmig commented 4 months ago

We are running increasingly into those issues at my company, so my plan is to carve out some time to work on this, both the atomic update and the ignore source files approaches, since I think both are relevant. Fyi 🙂

simonihmig commented 4 months ago

Here are two PRs for the "ignore source files" approach, ready to be reviewed:

Going to look into the atomic rollup update next week!