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

Run the clean plugin of addon-dev as late as possible #1229

Merged
merged 1 commit into from
Jul 5, 2022

Conversation

simonihmig
Copy link
Collaborator

@simonihmig simonihmig commented 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, 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 right before files are written, making the time window small enough to not cause any problems in practice. (successfully tested locally)

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.
@ef4
Copy link
Contributor

ef4 commented Jul 4, 2022

Thanks.

I wonder if, now that we have https://github.com/embroider-build/addon-blueprint/blob/d64ff367581eb95e5c92ea4d9799ddcfb44ef437/files/__addonLocation__/rollup.config.js we could just link people to there instead of maintaining this example file here too, and worrying about keeping them in sync.

@simonihmig
Copy link
Collaborator Author

Not sure I understand what you mean...

The change here is in the Addon class, that exposes the various rollup plugins in a somewhat simplified form (less config work). It's not just an example file, we are importing and using it in the actual addon (and as such also in the blueprint). Means once this is published and addons update the dependency, they will get the new behavior, without any "syncing".

Or do you think in hindsight, that the Addon class itself should be dropped, and addons should do the whole setup on their own?

@ef4
Copy link
Contributor

ef4 commented Jul 5, 2022

Oh, I'm sorry I misunderstood. I was imagining this was a change to https://github.com/embroider-build/embroider/blob/7bbd49c55a4b0f3bf4a1605f5307c412c089cfa0/packages/addon-dev/sample-rollup.config.js (which we could indeed remove from this repo in favor of the blueprint repo).

@ef4 ef4 merged commit b6a6997 into embroider-build:main Jul 5, 2022
@simonihmig simonihmig added the bug Something isn't working label Jul 5, 2022
@simonihmig simonihmig deleted the addondev-lateclean branch July 5, 2022 14:19
mansona added a commit that referenced this pull request May 24, 2023
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

Successfully merging this pull request may close these issues.

2 participants