Skip to content

Conversation

@ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented Oct 15, 2022

I will use this PR to propose a change and discuss it since it is easier to write code and comment in PRs.

Gist

  • class SideEffect
  • register/unregister a side effect on object/canvas/whatever level
  • flush side effects in the _set method and in other relevant places
  • should side effects run in initialization?

Motivation

  • history (undo/redo) discussed by the team - the dev could register as many side effects they want and trigger custom logic
  • replacing stateful Remove stateful concept ( without passing for deprecation first ). #7920
  • the need to run side effects and block them at runtime
  • a cleaner way to write side effects - move things like dimensionsAffectingProps under a class that contains the logic as well, all under one roof. Easier to read, develop and debug.
  • something similar to events - you can register/unregister a side effect, control it, create more. Eliminates the need to subclass, Easier to control behavior and easier to understand I hope.
  • fix(polyline): stroke bounding rect calculation typescript #8344 (review) and others regarding what side effects should or shouldn't run by default.
  • use instead of events in cases in which we don't want off to deregister the event and kill the side effect.
  • perf: using set({ ... }) invokes side effects for each key. That is suboptimal and can be addressed using side effects.
  • since using set({ ... }) invokes side effects for each key I can think of an edge case in which passing the same object with keys ordered differently will result in a different effect

Comments

I think this could be part of v6. No rush. As I wrote, this is a discussion that might turn into a PR.

Usage

class Polyline {
  constructor(...) {
     this.registerSideEffect(
          new SideEffect(
            'StrokeBBoxSideEffect', 
            ['strokeWidth', 'points', ...], 
           () => this.setDimensions()
      ))
  }

  flushSideEffects(key, value) {
    this.sideEffects.forEach(effect => effect.invoke(key, value))
  }
}

const lazyPolyline = new Polyline(...);
lazyPolyline.set('strokeWidth', 10) // sets dimensions
lazyPolyline.getSideEffect('StrokeBBoxSideEffect').disable()
lazyPolyline.set('strokeWidth', 20) // does nothing
lazyPolyline.unregisterSideEffect('StrokeBBoxSideEffect')
lazyPolyline.set('strokeWidth', 30) // does nothing for good

const shouldRunDimensionsEffect = !lazyPolyline.getSideEffect('StrokeBBoxSideEffect').isEqual({ strokewidth: 10, strokeUniform: false, ... })

@ShaMan123 ShaMan123 requested a review from asturur October 15, 2022 23:46
@github-actions
Copy link
Contributor

github-actions bot commented Oct 15, 2022

Coverage after merging side-effects into master will be

82.39%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
HEADER.js54.90%48.15%0%65.22%105, 105, 105, 12, 14, 14, 14, 14, 14, 16–17, 21, 21–22, 22, 22, 24, 24, 26, 28, 30–31
src
   cache.ts97.06%90%100%100%57
   canvas.class.ts93.44%90.36%94.23%95.58%1077, 1077–1078, 1081, 1101, 1101, 1136, 1169–1170, 1198–1199, 1232, 1240, 1350–1351, 1353–1354, 1374–1375, 1533, 1538, 1548, 1552, 485–486, 491, 500, 649–651, 696–697, 761–762, 765, 767, 814–816, 858, 863–864, 892–893
   config.ts77.27%66.67%66.67%84.62%130, 138–140, 151–153
   constants.ts100%100%100%100%
   intersection.class.ts100%100%100%100%
   pattern.class.ts92.19%85.71%100%96.30%110, 116, 127, 136, 88
   point.class.ts100%100%100%100%
   shadow.class.ts95.95%90%100%100%172, 239, 8
   static_canvas.class.ts90.10%83.79%96.70%92.75%1152–1153, 1153, 1153–1154, 1288, 1354–1355, 1358, 1407–1408, 1501, 1516, 1520, 1546–1547, 1576–1577, 1610–1611, 1652–1653, 1656, 1658, 1658, 1658, 1658, 1662, 1662, 1662–1664, 1686–1687, 1728–1729, 1732, 1734, 1734, 1734, 1734, 1738, 1738, 1738–1740, 1813, 1813–1814, 1873, 1875, 1875, 1875, 1875, 1875–1876, 1879–1880, 1880, 1880–1881, 1884, 1884, 1884, 1886, 1889, 1895, 1897–1898, 1898, 1898, 1901–1902, 1902, 1902, 1905, 278–279, 281–282, 284–285, 298–299, 301–302, 616, 642, 698–701, 901
src/brushes
   base_brush.class.ts100%100%100%100%
   circle_brush.class.ts2.99%0%0%3.92%100–101, 103, 105–107, 116, 116, 116, 118, 120, 122–124, 126–129, 137, 144, 146, 26, 31–32, 40–44, 48–52, 59–62, 70–74, 76, 84, 84, 84, 84, 84–85, 87, 87, 87–90, 92
   pattern_brush.class.ts5.26%0%0%8.33%16, 20–23, 25–26, 26, 26–29, 37–38, 40, 44, 55, 55, 55, 63–65, 65, 65, 72–73, 75–76, 76, 80
   pencil_brush.class.ts91.95%85.42%100%93.69%125–126, 155, 155–157, 279, 283, 288–289, 71–72, 87–88
   spray_brush.class.ts2.30%0%0%3.08%102–104, 106–107, 115, 115, 115, 115, 115–116, 118–119, 126–127, 129, 131–135, 144, 148–149, 149, 157, 157, 157–160, 162–165, 169–170, 172, 174–177, 180, 187–188, 190, 192–193, 195, 202–203, 205–206, 209, 209, 216, 216, 220, 25–26, 28–30, 30, 30–32, 36, 45, 52, 59, 66, 73, 80, 92–94
src/color
   color.class.ts91.67%84.51%100%94.44%325–326, 330–331, 334–335, 41, 45, 72–73, 73, 75, 75, 75–76, 78–79
   color_map.ts100%100%100%100%
   constants.ts100%100%100%100%
   index.ts100%100%100%100%
   util.ts100%100%100%100%
src/controls
   control.class.ts90.24%81.48%86.67%97.50%210, 304, 304, 348, 372, 6
   controls.actions.ts76.80%69.20%93.75%80.67%161, 163, 163, 163, 165, 167, 26, 28, 28, 28, 290–293, 321, 323, 330–331, 333, 333–334, 336–337, 341, 373, 375, 382–383, 385, 385–386, 388–389, 393, 421–422, 424, 426, 431, 434, 434, 434–435, 435, 435, 437, 437, 437–438, 438, 438, 441, 441, 441–442, 442, 442, 475–476, 478, 480, 485, 488, 488, 488–489, 489, 489, 491, 491, 491–492, 492, 492, 495, 495, 495–496, 496, 496, 520–522, 528, 528, 528–529, 532–535, 537, 537, 537–539, 539, 539–541, 543, 543, 543–545, 545, 545–546, 551, 551, 551–552, 554, 556–558, 57, 589–590, 592–594, 641–643, 8, 841
   controls.render.ts85.11%84.78%100%84.78%23, 27, 42–49, 58–59, 82, 86
   default_controls.ts94.29%50%100%100%115, 83
src/filters
   2d_backend.class.ts96.43%83.33%100%100%78
   WebGLProbe.ts40%40%57.14%34.78%38–39, 49–53, 61, 64, 66, 66, 66–67, 67, 67–70, 72, 74, 78
   base_filter.class.ts28.90%31.82%36.36%26.17%100–102, 102, 102–103, 112–116, 116, 116–117, 124–125, 125, 125–128, 143, 159, 169–174, 178, 181, 181, 181–184, 184, 184–185, 185, 188–189, 195, 204–205, 210–214, 257–260, 273, 273, 273–274, 276, 292–294, 294, 294, 294, 294–295, 297, 299–300, 306–307, 309–311, 315–316, 318, 322–324, 328, 332, 352, 352, 352–356, 393, 78, 78, 78–79, 79, 79–80, 80, 80–81, 86–89, 89, 89–90, 99

@ShaMan123
Copy link
Contributor Author

ShaMan123 commented Oct 19, 2022

I propose that Object.set returns an object/class instance:

class EffectsFlusher {
  constructor(effects: SideEffect[])

  // invokes side effects with options passed from set
  flush(options) {
     // dismisses side effects that should run only once (dismissible flag)
  }

  // did the set operation change the instance
  changed(): boolean

}
const effects = obj.set({ ... });
effects.changed() ? true: false;
// update state
effects.flush();

// short syntax
obj.set({ ... }).flush()

Then it is possible to run many set operation and concat the effects, flushing once at the end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants