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

feat: use PubSub Service singleton to subscribe to any SlickEvent #1248

Merged
merged 2 commits into from
Dec 6, 2023

Conversation

ghiscoding
Copy link
Owner

@ghiscoding ghiscoding commented Dec 6, 2023

  • by modifying the SlickEvent class to also publish an event when a PubSub Service is provided to a SlickEvent, we can avoid having our previous monkey patch of subscribing to all SlickGrid/DataView events to then dispatch event, this acted like a middleware, we can avoid all of that middleware with this new approach and publish right away via the PubSub when provided
  • also removed defaultComponentEventPrefix, defaultSlickgridEventPrefix grid options since they will not be used anymore
  • more info below

With this PR, we merged the SlickEvent with our own PubSub Service (which uses a CustomEvent), this makes it easier to maintain and requires less monkey patch code. The reason we use our own PubSub is because it makes it easier to communicate with Component based frameworks (used in Angular-Slickgrid, Slickgrid-React, ...). Note that this only applies to SlickGrid/DataView events since all other extensions (plugins/controls) were already using the PubSub through grid options or extension configurations. Note that you can still use SlickGrid .subscribe on SlickEvent but please remember to .unsubscribe each of them if you use that approach (which is not needed with a PubSub since you only have to do that once at a global level).

Also note that our PubSub Service is a JS CustomEvent based, so all the data is only available through the .eventDetail property of a CustomEvent. We use our PubSub to publish all events in the project, however the SlickGrid/DataView are the only events following this structure { eventData: SlickEventData; args: any; }, while all other extensions also publish events but the arguments are directly in the root (args).

What was the monkey patch anyway? The previous patch required to loop through all SlickGrid/DataView events (SlickEvent), we were then subscribing to each of them and whenever they were dispatching an event (through SlickEvent notify), we were then using our own PubSub Service to publish the event (similar to a middleware would do). As you can see, that was quite a monkey patch, the new approach simply adds an optional PubSub Service reference to the SlickEvent, allowing us to publish right away without the monkey patch or middleware.

- by modifying the SlickEvent class to also publish an event when a PubSub Service is provided to a SlickEvent, we can avoid having our previous monkey patch of subsribing to all SlickGrid/DataView events to then dispatch event, we can avoid all of that and publish right away
Copy link

codecov bot commented Dec 6, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (74fa5b5) 86.18% compared to head (745db9a) 86.19%.

Files Patch % Lines
packages/common/src/core/slickGrid.ts 92.86% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next    #1248      +/-   ##
==========================================
+ Coverage   86.18%   86.19%   +0.01%     
==========================================
  Files         197      197              
  Lines       21388    21391       +3     
  Branches     7099     7098       -1     
==========================================
+ Hits        18432    18435       +3     
  Misses       2956     2956              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ghiscoding ghiscoding merged commit 388bd11 into next Dec 6, 2023
5 checks passed
@ghiscoding ghiscoding deleted the feat/slickevent-with-pubsub-service branch December 6, 2023 15:49
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