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

avoid toolbar side effects #266

Merged
merged 3 commits into from
Mar 23, 2020
Merged

avoid toolbar side effects #266

merged 3 commits into from
Mar 23, 2020

Conversation

sgratzl
Copy link
Member

@sgratzl sgratzl commented Mar 18, 2020

closes #262

prerequisites:

  • branch is up-to-date with the branch to be merged with, i.e. develop
  • build is successful
  • code is cleaned up and formatted
  • tested with Firefox 52, Firefox 57+, Chrome 64+, MS Edge 16+

Summary

avoid side effects in the module for caching toolbar actions from columns. When developing toolbar addons one could experience caching issues

@sgratzl sgratzl requested a review from thinkh March 18, 2020 17:27
@sgratzl sgratzl linked an issue Mar 20, 2020 that may be closed by this pull request
@sgratzl sgratzl added this to Needs review in lineup Mar 20, 2020
@thinkh thinkh added the lineup: v4 All issues related to LineUp v4 label Mar 20, 2020
@thinkh
Copy link
Member

thinkh commented Mar 20, 2020

@sgratzl Is there a way to test it or what I should have a look it?

@sgratzl
Copy link
Member Author

sgratzl commented Mar 20, 2020

to see an effect you would need a project where you create a new column type with a custom toolbar annotation. A running example where the LineUp.js library is loaded and using the new column type. Then when adding a new option to the toolbar and just hot reloading the custom class without loading lineupjs you should see that still the old toolbar is used, since the toolbar options were cached in the library instance of LineUp.js instead of the current LineUp instance.

Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification.

An initial example would be https://codesandbox.io/s/empty-cherry-ynh2f

@thinkh thinkh merged commit 6f83c31 into lineup-v4 Mar 23, 2020
lineup automation moved this from Needs review to Done Mar 23, 2020
@thinkh thinkh deleted the sgratzl/module_sideeffect branch March 23, 2020 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lineup: v4 All issues related to LineUp v4
Projects
No open projects
lineup
  
Done
Development

Successfully merging this pull request may close these issues.

toolbar cache should be instance based only
2 participants