From a3d29cf60002869679d58259b9f9d2b2758bbcec Mon Sep 17 00:00:00 2001 From: 100pah Date: Tue, 14 Jul 2020 18:09:46 +0800 Subject: [PATCH] feature: add test case for id duplicated. --- src/model/Global.ts | 2 +- src/util/model.ts | 32 +++++++++++-------- test/lib/testHelper.js | 19 +++++++++--- test/option-replaceMerge.html | 58 +++++++++++++++++++++++++++++++++-- 4 files changed, 90 insertions(+), 21 deletions(-) diff --git a/src/model/Global.ts b/src/model/Global.ts index ad02e72f8..5fd20d9f4 100644 --- a/src/model/Global.ts +++ b/src/model/Global.ts @@ -384,7 +384,7 @@ class GlobalModel extends Model { let metNonInner = false; for (let i = realLen - 1; i >= 0; i--) { // Remove options with inner id. - if (opts[i] && !modelUtil.isIdInner(opts[i])) { + if (opts[i] && !modelUtil.isComponentIdInternal(opts[i])) { metNonInner = true; } else { diff --git a/src/util/model.ts b/src/util/model.ts index 7e21a2eea..e0939a91f 100644 --- a/src/util/model.ts +++ b/src/util/model.ts @@ -56,6 +56,8 @@ import { isNumeric } from './number'; */ const DUMMY_COMPONENT_NAME_PREFIX = 'series\0'; +const INTERNAL_COMPONENT_ID_PREFIX = '\0_ec_\0'; + /** * If value is not array, then translate it to array. * @param {*} value @@ -242,8 +244,8 @@ function mappingToExistsInNormalMerge( // Can not match when both ids existing but different. && existing && (existing.id == null || cmptOption.id == null) - && !isIdInner(cmptOption) - && !isIdInner(existing) + && !isComponentIdInternal(cmptOption) + && !isComponentIdInternal(existing) && keyExistAndEqual('name', existing, cmptOption) ) { result[i].newOption = cmptOption; @@ -264,7 +266,7 @@ function mappingToExistsInNormalMerge( * Mapping to exists for merge. * The mode "replaceMerge" means that: * (1) Only the id mapped components will be merged. - * (2) Other existing components (except inner compoonets) will be removed. + * (2) Other existing components (except internal compoonets) will be removed. * (3) Other new options will be used to create new component. * (4) The index of the existing compoents will not be modified. * That means their might be "hole" after the removal. @@ -286,20 +288,20 @@ function mappingToExistsInReplaceMerge( // contains elided items, which will be ommited. for (let index = 0; index < existings.length; index++) { const existing = existings[index]; - let innerExisting: T; + let internalExisting: T; // Because of replaceMerge, `existing` may be null/undefined. if (existing) { - if (isIdInner(existing)) { - // inner components should not be removed. - innerExisting = existing; + if (isComponentIdInternal(existing)) { + // internal components should not be removed. + internalExisting = existing; } - // Input with inner id is allowed for convenience of some internal usage. + // Input with internal id is allowed for convenience of some internal usage. // When `existing` is rawOption (see `OptionManager`#`mergeOption`), id might be empty. if (existing.id != null) { existingIdIdxMap.set(existing.id, index); } } - result.push({ existing: innerExisting }); + result.push({ existing: internalExisting }); } // Mapping by id if specified. @@ -350,7 +352,7 @@ function mappingByIndexFinally( return; } - // Find the first place that not mapped by id and not inner component (consider the "hole"). + // Find the first place that not mapped by id and not internal component (consider the "hole"). let resultItem; while ( // Be `!resultItem` only when `nextIdx >= mappingResult.length`. @@ -368,7 +370,7 @@ function mappingByIndexFinally( && !keyExistAndEqual('id', cmptOption, resultItem.existing) ) || resultItem.newOption - || isIdInner(resultItem.existing) + || isComponentIdInternal(resultItem.existing) ) ) { nextIdx++; @@ -414,6 +416,7 @@ function makeIdAndName( each(mapResult, function (item) { const opt = item.newOption; + // Force ensure id not duplicated. assert( !opt || opt.id == null || !idMap.get(opt.id) || idMap.get(opt.id) === item, 'id duplicates: ' + (opt && opt.id) @@ -511,12 +514,15 @@ export function isNameSpecified(componentModel: ComponentModel): boolean { * @param {Object} cmptOption * @return {boolean} */ -export function isIdInner(cmptOption: MappingExistingItem): boolean { +export function isComponentIdInternal(cmptOption: MappingExistingItem): boolean { return cmptOption && cmptOption.id != null - && makeComparableKey(cmptOption.id).indexOf('\0_ec_\0') === 0; + && makeComparableKey(cmptOption.id).indexOf(INTERNAL_COMPONENT_ID_PREFIX) === 0; } +export function makeInternalComponentId(idSuffix: string) { + return INTERNAL_COMPONENT_ID_PREFIX + idSuffix; +} export function setComponentTypeToKeyInfo( mappingResult: MappingResult, diff --git a/test/lib/testHelper.js b/test/lib/testHelper.js index f5a5f0fb9..88dc8799c 100644 --- a/test/lib/testHelper.js +++ b/test/lib/testHelper.js @@ -275,10 +275,19 @@ * `testHelper.printAssert` can be called multiple times for one chart instance. * For each call, one result (fail or pass) will be printed. * - * @param chart {EChartsInstance} + * @param chartOrDomId {EChartsInstance | string} * @param checkFn {Function} param: a function `assert`. */ - testHelper.printAssert = function (chart, checkerFn) { + testHelper.printAssert = function (chartOrDomId, checkerFn) { + var hostDOMEl; + var chart; + if (typeof chartOrDomId === 'string') { + hostDOMEl = document.getElementById(chartOrDomId); + } + else { + chart = chartOrDomId; + hostDOMEl = chartOrDomId.getDom(); + } var failErr; function assert(cond) { if (!cond) { @@ -292,7 +301,7 @@ console.error(err); failErr = err; } - var printAssertRecord = chart.__printAssertRecord || (chart.__printAssertRecord = []); + var printAssertRecord = hostDOMEl.__printAssertRecord || (hostDOMEl.__printAssertRecord = []); var resultDom = document.createElement('div'); resultDom.innerHTML = failErr ? 'checked: Fail' : 'checked: Pass'; @@ -305,12 +314,12 @@ 'color: ' + (failErr ? 'red' : 'green') + ';', ].join(''); printAssertRecord.push(resultDom); - chart.getDom().appendChild(resultDom); + hostDOMEl.appendChild(resultDom); relayoutResult(); function relayoutResult() { - var chartHeight = chart.getHeight(); + var chartHeight = chart ? chart.getHeight() : hostDOMEl.offsetHeight; var lineHeight = Math.min(fontSize + 10, (chartHeight - 20) / printAssertRecord.length); for (var i = 0; i < printAssertRecord.length; i++) { var record = printAssertRecord[i]; diff --git a/test/option-replaceMerge.html b/test/option-replaceMerge.html index e525e7a67..1c8ff5f1d 100644 --- a/test/option-replaceMerge.html +++ b/test/option-replaceMerge.html @@ -44,11 +44,12 @@ under the License.
-
+
+
@@ -627,7 +628,7 @@ under the License. }] }; - var chart = testHelper.create(echarts, 'main_replaceMerge_inner_and_other_cmpt_not_effect', { + var chart = testHelper.create(echarts, 'main_replaceMerge_internal_and_other_cmpt_not_effect', { title: [ 'replaceMerge: inner not effect', 'click "setOption": a dataZoom.slider added', @@ -1004,6 +1005,59 @@ under the License. + + + + + + + + + + + -- GitLab