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

Always notify watchers of plugin updates #2508

Merged

Conversation

dcalhoun
Copy link
Contributor

During plugin updates, each plugin.onUnload is invoked regardless of whether the plugin changed. This commit ensures that each plugin.onApp is invoked once all plugins have been updated. Fixes #2415.

During plugin updates, each `plugin.onUnload` is invoked regardless of
whether the plugin changed. This commit ensures that each `plugin.onApp`
is invoked once all plugins have been updated. Fixes vercel#2415.
@dcalhoun
Copy link
Contributor Author

@chabou You originally stated:

onUnload method should be called only on removed plugins imo.

I agreed with that originally, but as I thought about it more I'm not sure that would work. For example, if a plugin changes, the plugin's onUnload and onApp need to be invoked so that Hyper ensures that it executes the plugin's latest setup.

In theory, we could change Hyper to only invoke onUnload for removed/changed plugins and only invoke onApp for added/changed plugins, but things quickly increased in complexity. In the end, I felt sticking with calling onUnload and onApp for every plugin was the most straightforward solution.

@dcalhoun
Copy link
Contributor Author

The PR is ready to be merged. Please let me know if you have any feedback.

@dcalhoun
Copy link
Contributor Author

@chabou @rauchg @leo @matheuss sorry to bother, but is there anything I can do to help move this PR forward?

@leo leo merged commit bbb1cae into vercel:canary Jan 4, 2018
@leo
Copy link
Contributor

leo commented Jan 4, 2018

Thank you!

@dcalhoun dcalhoun deleted the always-notify-watchers-of-plugin-updates branch January 4, 2018 13:56
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.

None yet

3 participants