Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rollup clean plugin causes noisy build errors #32

Open
ef4 opened this issue Jul 1, 2022 · 9 comments
Open

Rollup clean plugin causes noisy build errors #32

ef4 opened this issue Jul 1, 2022 · 9 comments
Labels
bug Something isn't working

Comments

@ef4
Copy link
Contributor

ef4 commented Jul 1, 2022

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
Copy link
Collaborator

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
Copy link
Contributor Author

ef4 commented Jul 4, 2022

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

simonihmig added a commit to simonihmig/embroider that referenced this issue Jul 4, 2022
This attempts to fix the problem described in embroider-build/addon-blueprint#32.

While it is not the perfect solution like a truly atomic build update, it seems "good enough". Previously the cleanup would happen at the earliest point in time, at [`buildStart`](https://github.com/vladshcherbin/rollup-plugin-delete/blob/master/src/index.js#L5), making the time window large enough for Ember CLI to see the transient build output in an inconsistent state. Now it happens at the *latest* possible time, at [`generateBundle`](https://rollupjs.org/guide/en/#generatebundle) right before files are written, making the time window small enough to not cause any problems in practice.
@simonihmig
Copy link
Collaborator

Fix: embroider-build/embroider#1229

@simonihmig
Copy link
Collaborator

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

@simonihmig
Copy link
Collaborator

I encountered some related problems again:

  • I was using rollup-plugin-scss, and it seems my change in Run the clean plugin of addon-dev as late as possible embroider#1229 caused the problem, as it seems now wipes out things after the Sass has been processed and the CSS written to disk, i.e. no CSS is found
  • Besides that the CSS is missing, it seems also build artefacts are written to disk at different points in time, which cause multiple re-builds shortly after another, which is annoying

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

@simonihmig simonihmig reopened this Nov 11, 2022
@simonihmig simonihmig added the bug Something isn't working label Nov 11, 2022
@mansona
Copy link
Member

mansona commented Apr 27, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

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

Going to look into the atomic rollup update next week!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants