[GitHub] [incubator-echarts] pissang commented on a change in pull request #13358: Custom morph2

[GitBox <gi...@apache.org>]

pissang commented on a change in pull request #13358:
URL: https://github.com/apache/incubator-echarts/pull/13358#discussion_r508959276



##########
File path: src/data/Source.ts
##########
@@ -180,6 +182,35 @@ class SourceImpl {
         this.metaRawOption = fields.metaRawOption;
     }
 
+    /**
+     * When expose the source to thrid-party transform, it probably better to
+     * freeze to make sure immutability.
+     * If a third-party transform modify the raw upstream data structure, it might bring about
+     * "uncertain effect" when using multiple transforms with different combinations.
+     *
+     * [Caveat]
+     * `OptionManager.ts` have perform `clone` in `setOption`.
+     * The original user input object should better not be frozen in case they
+     * make other usages.
+     */
+    freeze() {
+        assert(sourceFormatCanBeExposed(this));
+
+        const data = this.data as OptionSourceDataArrayRows;
+        if (this.frozen || !data || !isFunction(Object.freeze)) {
+            return;
+        }
+        // @ts-ignore
+        this.frozen = true;
+        // PENDING:
+        // There is a flaw that there might be non-primitive values like `Date`.
+        // Is it worth handling that?
+        for (let i = 0; i < data.length; i++) {
+            Object.freeze(data[i]);
+        }

Review comment:
       I think it's not worth. The performance of Object.freeze varies on different browsers.
   
   https://jsfiddle.net/rgp6oudL/
   
   I tested it on safari and it's quite slow. 

##########
File path: src/data/DataDiffer.ts
##########
@@ -61,107 +86,215 @@ class DataDiffer<Ctx = unknown> {
 
         // Visible in callback via `this.context`;
         this.context = context;
+
+        this._diffModeMultiple = diffMode === 'multiple';
     }
 
     /**
      * Callback function when add a data
      */
-    add(func: DiffCallbackAdd): DataDiffer {
+    add(func: DiffCallbackAdd): this {
         this._add = func;
         return this;
     }
 
     /**
      * Callback function when update a data
      */
-    update(func: DiffCallbackUpdate): DataDiffer {
+    update(func: DiffCallbackUpdate): this {
         this._update = func;
         return this;
     }
 
+    /**
+     * Callback function when update a data and only work in `cbMode: 'byKey'`.
+     */
+    updateManyToOne(func: DiffCallbackUpdateManyToOne): this {
+        this._updateManyToOne = func;
+        return this;
+    }
+
+    /**
+     * Callback function when update a data and only work in `cbMode: 'byKey'`.
+     */
+    updateOneToMany(func: DiffCallbackUpdateOneToMany): this {
+        this._updateOneToMany = func;
+        return this;
+    }
+
     /**
      * Callback function when remove a data
      */
-    remove(func: DiffCallbackRemove): DataDiffer {
+    remove(func: DiffCallbackRemove): this {
         this._remove = func;
         return this;
     }
 
     execute(): void {
+        this[this._diffModeMultiple ? '_executeByKey' : '_executeByIndex']();
+    }
+
+    private _executeByIndex(): void {

Review comment:
       I'm misled by the name `ByIndex`. Do you mind explaining the meaning of this name?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@echarts.apache.org
For additional commands, e-mail: commits-help@echarts.apache.org