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

Enable addons to be rebuilt #297

Merged
merged 5 commits into from
Aug 26, 2019
Merged

Conversation

thoov
Copy link
Collaborator

@thoov thoov commented Jul 30, 2019

A POC of how addons could be selectively rebuilt if Broccoli passed change information into the plugin. The goal of this PR is to get overall feedback on the strategy and to confirm this is a viable path forward. I will add tests once we work out the higher level vision of this.

Overall Strategy:

  • WaitForTrees asks broccoli for change information about its inputs (not yet added to broccoli, a POC on broccoli's side can be found here: [FEATURE] Input node change tracking broccolijs/broccoli#419).
  • WaitForTrees translates that information down to the build callback in compat-addon
  • The build callback uses tree-sync to conditionally "sync" to the workspaceDir based on if the addon has changed (for the first build all addons are considered "changed")

Perf:

Measuring the execution time of the build callback for a brand new app:

With change tracking: ~50ms
Without: ~75ms

I have not tested this in a large app but change tracking should stay relatively constant were as without change tracking will obviously grow with the number of addons.

Limitations:

This conditional checking would only be available for those that are using the version of broccoli were this is enabled. These changes are backwards compatible however there would be performance implications for those not on it.

Open Questions for Reviewers:

  • How should plugins and broccoli communicate change? In this POC broccoli passes change info down to the plugin or should it be where the plugin "calls" up and asks broccoli for this information?
  • Should we only enable rebuilding for those only on the latest broccoli?

#253

A POC of how addons could be selectively rebuilt if Broccoli passed
change information into the plugin.
@thoov
Copy link
Collaborator Author

thoov commented Jul 30, 2019

cc: @stefanpenner & @ef4

Copy link
Contributor

@ef4 ef4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks pretty good.

Regarding the performance hit for older versions of broccoli, I think if we make this code check mayRebuild from here it will already have the right effect. Any addons that have mayRebuild false would not get synced and would only get copied on the first build.

packages/compat/src/compat-addons.ts Outdated Show resolved Hide resolved
@thoov thoov changed the title [WIP] Enable addons to be rebuilt Enable addons to be rebuilt Aug 22, 2019
@thoov
Copy link
Collaborator Author

thoov commented Aug 22, 2019

@ef4 / @stefanpenner could you guys review again?

@ef4
Copy link
Contributor

ef4 commented Aug 26, 2019

Looks good to me, thanks.

@ef4 ef4 merged commit 51b5062 into embroider-build:master Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants