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

[감하영] datasets의 default 값 설정 & datasets의 ID 자동 추가 & datasets type 및 구조 validation 그 외 #5

Merged
merged 9 commits into from Sep 1, 2021

Conversation

iamhayoung
Copy link
Collaborator

@iamhayoung iamhayoung commented Aug 26, 2021

작업 내용

  • 차트 데이터를 담는 props인 datasets의 Default 값 설정
  • datasets의 개별 아이템에 ID 자동 추가
  • datasets 데이터 type 및 structure 체크
  • 라이브러리명 변경 (EasyChartBoard → VuetifulBoard)

상세 내용

1. 차트 데이터를 담는 props인 datasets의 Default 값 설정 (2021/8/29 수정)

차트를 출력하는 최소한의 데이터로, 아래의 정보를 설정

  • id : 차트의 unique ID
  • chartInfo.id: chartInfos를 for문을 돌릴 때 key값으로 사용할 unique ID
  • chartInfo.series : 차트의 실제 데이터를 가리킴. default로 빈 배열을 지정.
  • chartInfo.options.chart.type : 차트의 타입(line, bar...etc)을 가리킴. default로 null을 지정.
  • gridInfo.id: gridInfos로 for문을 돌릴 때 key값으로 사용할 unique ID
// src/components/EasyChartBoard.vue

props: {
  datasets: {
    type: Array,
    require: true,
    default: function() {   // datasets가 존재하지 않을 때 빈 차트를 보여주기 위해 Default 값 설정
      return [
        {
          id: this.$uuid.v4(),    //  차트의 unique ID
          chartInfo: {
            id: this.$uuid.v4(),   // `chartInfos`로 for문을 돌릴 때 key값으로 사용할 unique ID
            series: [],
            options: {
              chart: {
                type: null,
              },
            },
          },
          gridInfo: {
             id: this.$uuid.v4(),   // `gridInfos`로 for문을 돌릴 때 key값으로 사용할 unique ID
          },
        },
      ];
    },
  },
},

결과 화면

아래의 코드와 같이 EasyChartBoard 컴포넌트에 datasets props을 설정하지 않은 경우,
default 값이 적용되어 하나의 빈 차트가 출력됨

<!-- docs/.vuepress/components/EasyChartBoardPlayground.vue -->

<template>
  <div>
    <easy-chart-board />
  </div>
</template>

<script>
export default {
}
</script>

image

2. datasets의 개별 아이템에 ID 자동 추가 (2021/8/29 추가)

EasyChartBoard의 datasets에 데이터를 넘길 때,
ID를 담아주지 않아도 라이브러리 측에서 ID를 자동으로 추가해주는 로직 구현.
(사용자 측에서 매번 ID를 추가해줘야하는 번거로움 해소)

<!-- src/components/EasyChartBoard.vue -->

<template>
  <div v-if="chartInfos.length"> // chartInfos에 값이 존재할 때 차트를 렌더링
    <apex-charts
      v-for="item in chartInfos" // 차트 렌더링 시, chartInfos의 데이터를 이용
      :key="item.id"
      :type="item.options.chart.type"
      :series="item.series"
      :options="item.options"
    />
  </div>
</template>

<script>
export default {
  props: {
    datasets: {
      type: Array,
      require: true,
      default: function() {   // datasets가 존재하지 않을 때 빈 차트를 보여주기 위해 Default 값 설정
        return [
          {
            id: this.$uuid.v4(),   // 차트의 unique ID
            chartInfo: {
              id: this.$uuid.v4(),   // `chartInfos`로 for문을 돌릴 때 key값으로 사용할 unique ID
              series: [],
              options: {
                chart: {
                  type: null,
                },
              },
            },
            gridInfo: {
              id: this.$uuid.v4(),   // `gridInfos`로 for문을 돌릴 때 key값으로 사용할 unique ID
            },
          },
        ];
      },
    },
  },
  data() {
    return {
      chartInfos: {
        type: Array,
      },
      gridInfos: {
        type: Array,
      },
    };
  },
methods: {
    // ....중략

    addUniqueId() {
      // addUniqueId: Unique ID를 추가해주는 함수
      return this.datasets.map(item => {
        item.id = this.$uuid.v4();
        item.chartInfo.id = this.$uuid.v4();
        item.gridInfo.id = this.$uuid.v4();
        return item;
      });
    },
  },
  mounted() {
    const datasets = this.addUniqueId();  // addUniqueId 실행 후 결과 값을 `datasets`라는 변수에 담음

    // Unique ID가 추가된 datasets를 `chartInfos`, `gridInfos`로 분리
    this.chartInfos = datasets.map(item => item.chartInfo);
    this.gridInfos = datasets.map(item => item.gridInfo);
  },
};
</script>
<!-- Usage -->
<!-- docs/.vuepress/components/EasyChartBoardPlayground.vue -->

<template>
  <div>
    <easy-chart-board
      :datasets="[
        {
          // ID 추가하지 않음
          chartInfo: {}
          gridInfo: {}
        },
        {
          // ID 추가하지 않음
          chartInfo: {}
          gridInfo: {}
        },
       ... 중략
    />
  </div>
</template>
... 중략

결과 화면

datasets가 존재할 때

위의 Usage의 코드 그대로 사용한 경우.
ID를 입력해주지 않았음에도 각 아이템에 ID가 추가되며 문제없이 차트가 출력된 모습.

image

datasets가 존재하지 않을 때

default에 지정해둔 값이 그대로 반영되면서 (ID 有) 빈 차트가 출력됨.

<!-- docs/.vuepress/components/EasyChartBoardPlayground.vue -->

<template>
  <div>
    <easy-chart-board />
  </div>
</template>

<script>
export default {
}
</script>

<style lang="scss" scoped></style>

image

3. datasets 데이터 type 및 structure 체크 (2021/8/29 추가)

차트의 경우, 차트 데이터에 따라 옵션이 천차만별이기 때문에,
최소한의 데이터인 Default 값을 기준으로 validation 함수 구현. (Default 정보는 위의 1번 항목 코드 및 아래 코드 참조)

차트 데이터인 datasets는 mount에 필요한 정보이기 때문에,
마운트 전 created()에서 validation 함수를 실행하여 타입 및 구조 체크.

validation 진행 후, mounted()에서 datasetschartInfos, gridInfos로 분리

// src/components/EasyChartBoard.vue

<script>
export default {
  props: {
    datasets: {
      type: Array,
      require: true,
      default: function() {
        return [
          {
            id: this.$uuid.v4(),
            chartInfo: {
              id: this.$uuid.v4(),
              series: [],
              options: {
                chart: {
                  type: null,
                },
              },
            },
            gridInfo: {
              id: this.$uuid.v4(),
            },
          },
        ];
      },
    },
  },
  data() {
    return {
      chartInfos: {
        type: Array,
      },
      gridInfos: {
        type: Array,
      },
    };
  },
  methods: {
    validateProps() {
      // validateProps: datasets의 구조 및 내부 데이터의 structure를 체크하는 함수
      for (const data of this.datasets) {
        const { chartInfo, gridInfo } = data;
        const { series, options } = chartInfo;
        const { chart } = options;

        // 최소한의 데이터인 Default 값을 기준으로 validation 함수 구현.
        // 조건에 부합하지 않을 시 에러 출력
        if (
          !(chartInfo?.constructor.name === 'Object') ||
          !(options?.constructor.name === 'Object') ||
          !(chart?.constructor.name === 'Object') ||
          !(gridInfo?.constructor.name === 'Object') ||
          !(series?.constructor.name === 'Array') ||
          !(!chart.type || chart.type.constructor.name === 'String')
        ) {
          console.error(
            '[EasyChartBoard warn]: Invalid datasets prop: Please check the type or structure of datasets prop.',
          );
        }
      }
    },
    addUniqueId() {
      // addUniqueId: Unique ID를 추가해주는 함수
      return this.datasets.map(item => {
        item.id = this.$uuid.v4();
        item.chartInfo.id = this.$uuid.v4();
        item.gridInfo.id = this.$uuid.v4();
        return item;
      });
    },
  },
  created() {
    // 마운트 전 validateProps 함수 실행
    this.validateProps();
  },
  mounted() {
    const datasets = this.addUniqueId(); // addUniqueId 실행 후 결과 값을 `datasets`라는 변수에 담음

    // Unique ID가 추가된 datasets를 `chartInfos`, `gridInfos`로 분리
    this.chartInfos = datasets.map(item => item.chartInfo);
    this.gridInfos = datasets.map(item => item.gridInfo);
  },
};
</script>

결과 화면

예시: gridInfo의 type이 Object가 아닐 때, 콘솔에 에러가 출력됨

<!-- docs/.vuepress/components/EasyChartBoardPlayground.vue -->

<template>
  <div>
    <easy-chart-board
      :datasets="[
        {
          chartInfo: {
               // ...중략
          }
          gridInfo: [],
        },
       ]"
    />
  </div>
</template>

image

@iamhayoung iamhayoung changed the base branch from master to develop August 27, 2021 00:05
@iamhayoung iamhayoung changed the title [감하영] 차트 데이터(datasets)의 default 값 설정 [감하영] 차트 데이터(datasets)의 default 값 설정, datasets의 ID 자동 추가 Aug 28, 2021
@iamhayoung iamhayoung changed the title [감하영] 차트 데이터(datasets)의 default 값 설정, datasets의 ID 자동 추가 [감하영] 차트 데이터(datasets)의 default 값 설정, datasets의 ID 자동 추가, datasets type & 구조 validation Aug 29, 2021
@iamhayoung iamhayoung changed the title [감하영] 차트 데이터(datasets)의 default 값 설정, datasets의 ID 자동 추가, datasets type & 구조 validation [감하영] datasets의 default 값 설정 & datasets의 ID 자동 추가 & datasets type 및 구조 validation Aug 29, 2021
@iamhayoung
Copy link
Collaborator Author

iamhayoung commented Aug 29, 2021

@ktseo41

보현님, 안녕하세요.
아래의 내용에 대해 구현 완료했습니다.

구현 세부사항에 관해서는 위의 본문에 기재하였습니다. (초 장문이라 죄송합니다..)
확인 및 검토 부탁드립니다.

리뷰 대기하는 동안 테마 관리 기능 관련해서 조사 및 구현 시작하겠습니다!
잘부탁드립니다.

구현 내용

  • datasets의 Default 값 설정 (수정사항 있음)
  • datasets의 개별 아이템에 ID 자동 추가 (신규 구현)
  • datasets 데이터 type 및 structure 체크 (신규 구현)

특이 사항

  • 8/27 금요일 중간 리뷰 회의에서 언급되었던, 아래의 건도 함께 반영하여 구현
    • grid-layout에서 grid의 사이즈나 위치 등이 변했을 때, 자식 요소의 세부적인 변화까지 감지하지 못하기 때문에, 데이터를 최상위 단위로 뽑아내기 위해 datasetschartInfosgridInfos로 분리
  • gridInfo validation 관련
    • gridInfo가 최소한 객체 형태인지만 체크함. (gridInfo 내부 값(x, y 등등)의 체크에 대해서는 @hxyxneee님이 구현 예정)

Copy link
Contributor

@ktseo41 ktseo41 left a comment

Choose a reason for hiding this comment

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

수고 많으셨습니다 !

validator 관련해서 리뷰를 남겼습니다. 최소한으로 하면 되는건데 그래도 약간의 고통이 따르네요. javascript가 엄격하지 않은 언어라서 오히려 사용에 조금 주의가 필요해서 그런 것 같습니다. 그래도 매력적인 언어라고 생각합니다 ㅎ

다시 한번 수고 많으셨습니다 ~!

@@ -1,11 +1,11 @@
<template>
<div>
<div v-if="chartInfos.length">
Copy link
Contributor

Choose a reason for hiding this comment

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

좋네용

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

감사합니다!

Comment on lines 51 to 70
validateProps() {
for (const data of this.datasets) {
const { chartInfo, gridInfo } = data;
const { series, options } = chartInfo;
const { chart } = options;

if (
!(chartInfo?.constructor.name === 'Object') ||
!(options?.constructor.name === 'Object') ||
!(chart?.constructor.name === 'Object') ||
!(gridInfo?.constructor.name === 'Object') ||
!(series?.constructor.name === 'Array') ||
!(!chart.type || chart.type.constructor.name === 'String')
) {
console.error(
'[EasyChartBoard warn]: Invalid datasets prop: Please check the type or structure of datasets prop.',
);
}
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

작성 수고 많으셨습니다 !

말씀하셨던대로 vue 기본 error 메세지가 아니라 custom 에러 메세지를 보여주는게 validator로도 가능한지 한번 찾아봐야겠네요.

Copy link
Contributor

Choose a reason for hiding this comment

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

요게 typeof 가 array랑 object 랑 똑같이 'object'를 return하기 때문에 typeof 쓸 때 항상 고민인데 객체는 toString method, 배열은 isArray를 사용하면 구분을 할 수 있습니다.

Object.prototype.toString.call(chartInfo) === "[object Object]" ||
Array.isArray(series) ||
// ... 동일

위처럼 constructor를 참조하는 경우는 아래처럼 추가한 class의 instance일 때는 사용할 수 없는 방법이겠네요

class Person{
  constructor(name){
    this.name = name
  }
}

const james = new Person('james')

james.constructor.name === 'Person' // true

저희 라이브러리를 사용하는 사람이 옵션값으로 class instance를 넘겨줄 일은 없겠지만 type validation을 하는 적절한 방법은 아니어서

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof 이 문서 참고하면

아래부분에 javascript type validation 함수도 추가해놔서 이거 사용해도 될 것 같습니다. ( mdn 공식 함수가 있는 줄 저도 이번에 처음알았습니다 ) 아니면 사실 toString도 충분해요!

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof#real-world_usage

위 함수를 사용하면 아래처럼 축약될 수 있을 것 같습니다.

Suggested change
validateProps() {
for (const data of this.datasets) {
const { chartInfo, gridInfo } = data;
const { series, options } = chartInfo;
const { chart } = options;
if (
!(chartInfo?.constructor.name === 'Object') ||
!(options?.constructor.name === 'Object') ||
!(chart?.constructor.name === 'Object') ||
!(gridInfo?.constructor.name === 'Object') ||
!(series?.constructor.name === 'Array') ||
!(!chart.type || chart.type.constructor.name === 'String')
) {
console.error(
'[EasyChartBoard warn]: Invalid datasets prop: Please check the type or structure of datasets prop.',
);
}
}
},
validateProps() {
for (const data of this.datasets) {
const { chartInfo, gridInfo } = data;
const { series, options } = chartInfo;
const { chart } = options;
if (
!(type(chartInfo) === 'object') ||
!(type(options) === 'object') ||
!(type(chart) === 'object') ||
!(type(gridInfo) === 'object') ||
!(type(series) === 'array') ||
!(!chart.type || type(chart) === 'string')
) {
console.error(
'[EasyChartBoard warn]: Invalid datasets prop: Please check the type or structure of datasets prop.',
);
}
}
},

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

보현님 상세한 자료와 예시 코드, 지적까지 너무 감사합니다🙇‍♀️

말씀해주신대로 class를 사용하는 경우엔 constructor.name을 이용한 로직을 사용하기 어렵겠네요!
제안 주신 Object.prototype.toString.call()를 이용하면
객체의 타입까지 구체적으로 알아볼 수 있다는 점을 새롭게 배울 수 있었습니다. 감사합니다!

Object.prototype.toString.call([]);  // '[object Array]'
Object.prototype.toString.call(new Date);  // '[object Date]'
Object.prototype.toString.call(/s/);  // '[object RegExp]'
Object.prototype.toString.call(null);  // '[object Null]'
Object.prototype.toString.call(1);  // '[object Number]'
Object.prototype.toString.call('abc');  // '[object String]'

Object.prototype.toString.call()MDN type 체크 예시 함수를 참고하여
isType이라는 타입 체크함수를 만들었습니다.

e4135a5

isType은 인자로 받은 데이터의 타입을 체크하고, 소문자 string으로 타입 형태를 반환합니다.
반환받은 값을 바탕으로 if문의 조건문을 거치게 되고
조건에 부합하지 않을 시에 에러를 출력하는 쪽으로 수정해보았습니다!

Comment on lines 71 to 78
addUniqueId() {
return this.datasets.map(item => {
item.id = this.$uuid.v4();
item.chartInfo.id = this.$uuid.v4();
item.gridInfo.id = this.$uuid.v4();
return item;
});
},
Copy link
Contributor

Choose a reason for hiding this comment

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

사용자가 id를 지정해줄 수도 있으니 아래처럼 id가 없을 때 지정하도록 해주면 좋을 것 같습니다

Suggested change
addUniqueId() {
return this.datasets.map(item => {
item.id = this.$uuid.v4();
item.chartInfo.id = this.$uuid.v4();
item.gridInfo.id = this.$uuid.v4();
return item;
});
},
addUniqueId() {
return this.datasets.map(item => {
if(item.id === undefined){
item.id = this.$uuid.v4();
}
// 혹은 item.id = item.id || this.$uuid.v4(); 인데 이러면 id를 0으로는 지정을 못하겠네요 🤔
// 대안 -> item.id = item.id !== undefined ? item.id : this.$uuid.v4();
// 나중에 Nullish coalescing operator 추가되면 -> item.id = item.id ?? this.$uuid.v4()
// 아래도 동일
item.id = this.$uuid.v4();
item.chartInfo.id = this.$uuid.v4();
item.gridInfo.id = this.$uuid.v4();
return item;
});
},

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

사용자가 ID를 지정했을 때는 사용자가 지정한 ID값을 사용할 수 있도록 해줘야하는군요!
놓치고 지나갈 뻔한 부분이었는데 감사합니다!

위의 제안해주신 코드 중에서 Nullish coalescing operator ??를 사용하는 것이 가장 합리적이라고 생각되어서
??를 사용하는 방향으로 수정해보았습니다. 04c114d

(나중에 Nullish coalescing operator 추가되면이라는 문구가 있어서
혹시 vue에서 호환되지 않나 싶었는데 사용해보니 문제없이 동작했습니다!
Nullish coalescing operator와 관련해서 무언가 우려하시고 계시는 것이 있을까요?)


결과 화면

아래와 같이 ID에 0을 설정한 경우,

<template>
  <div>
    <easy-chart-board
      :datasets="[
        {
          id: 0,
          chartInfo: {...},
          gridInfo: {...},
        },
        {
          chartInfo: {...},
          gridInfo: {...},
        },
        {
          id: 0,
          chartInfo: {...},
          gridInfo: {...},
        },
        ...중략
      ]"
    />
 </div>
</template>

id에 0을 지정해준 첫번째 아이템에는 id: 0이 잘 설정되었고
id를 설정해주지 않은 그 외 아이템에는 uuid로 발급된 unique id가 부여된 모습.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

JavaScript에 공식적으로 feature가 추가될때 몇가지 단계를 거쳐서 추가가 되는데, Nullish coalescing이 얼마전까지 공식으로 추가된 기능이 아니었어서 못쓰는줄 알았는데 찾아보니 올해 6월에 feature로 추가됐네요

근데 그래도 최신 브라우저 && 최신 버전 이어야 사용이 가능하고, Chrome의 경우는 JavaScript에 공식적으로 추가된 스펙이 아니더라도 미리 브라우저 사용할 수 있도록 지원하는 경우도 있어서 Chrome에서 잘돼도 주의해야하는걸로 알고 있습니다.

javascript.info에서도 이런 주의를 잘 표시해주고 있네요

Screen Shot 2021-08-31 at 9 31 16 PM

아시겠지만 caniuse에서 보니

Screen Shot 2021-08-31 at 9 35 42 PM

이고, 91% 브라우저에서 사용할 수 있다니 괜찮을 것도 같지만 이런 경우에는 babel 등을 사용해 이전 버전 브라우저에서도 호환될 수 있는 코드로 변환해주는 과정을 거치는게 낫습니다.

이제 그래도 IE가 공식적으로 중단돼서 얼마나 다행인지..

vue에서도 직접 사용할 때는 아래처럼 코드를 변형해서 사용해주고 있네요

Screen Shot 2021-08-31 at 9 38 11 PM

babel에서 이전 버전 브라우저에 호환되는 코드로 변형할 때 타겟 버전을 지정해주는걸로 알고 있는데 vue에서는 이 타겟이 어느버전인지는 모르겠네요

Copy link
Contributor

Choose a reason for hiding this comment

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

결론은 vue clie에서 알아서 잘해주고 있네요 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

오리엔테이션 날에 메디스트림에서는 최소 ie11까지 지원하는 방향으로 대응하고 있고
브라우저 호환성 체크를 중요시하고 있다는 것을 강조하신 기억이 납니다.

미처 짚고 넘어가지 못해 죄송합니다. 꼼꼼하게 챙기도록 노력하겠습니다
그리고 이렇게 먼저 자세하게 체크해주시고 레퍼런스를 참조해주셔서 너무 감사합니다!

vue cli의 @vue/babel-preset-app에서는 아래의 함수로 브라우저 타겟을 지정해주고 있는 듯 합니다.

function getWCTargets (targets) {
  // targeting browsers that at least support ES2015 classes
  // https://github.com/babel/babel/blob/v7.9.6/packages/babel-compat-data/data/plugins.json#L194-L204
  const allWCTargets = getTargets(
    {
      browsers: [
        'Chrome >= 46',
        'Firefox >= 45',
        'Safari >= 10',
        'Edge >= 13',
        'iOS >= 10',
        'Electron >= 0.36'
      ]
    },
    { ignoreBrowserslistConfig: true }
  )

  // use the intersection of browsers supporting Web Components and user defined targets config
  return getIntersectionTargets(targets, allWCTargets)
}

적어도 ES2015(ES6) 클래스를 지원하는 브라우저를 타겟(해당 브라우저들을 지원하라고 설정)한다는 코멘트가 적혀 있기 때문에
대다수의 ES6 문법을 지원하지 않는 IE는 위 리스트에 포함되지 않은 듯 합니다.

하지만 ?? 연산자는 그외 ES6 지원 브라우저들을 호환하는 과정에서 운좋게(?) 컴파일 된 것이 아닐까라고 추측하고 있습니다.

서론이 길었지만 제가 조사, 이해한 내용이 맞다면
보현님의 말씀대로 결론적으로 @vue/babel-preset-app에서 코드를 컴파일 해주었기 때문에
Nullish coalescing operator를 사용해도 좋다!고 이해해도 좋을까요?

아직 approve를 받지 못했는데 만약 Nullish coalescing operator을 사용해도 좋다면,
이 부분 외의 지적 사항은 모두 수정을 마쳤기 때문에 approve를 받을 수 있을까요?


⭐️ 추가:

이 부분을 조사하는 과정에서 vuetify는 polyfill없이 ie11을 지원하지 않는다는 것을 알게 되었습니다.
서비스단에서 별도로 ie11 호환을 위한 설정을 추가하겠습니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

target 브라우저 찾아주셔서 감사합니다. ECMAScript 대응 시트는 처음 보네요! approve로 늦게 바꿔 죄송합니다! 넵 Nullish coalescing operator는 그대로 사용해도 될 것 같습니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

저야말로 감사합니다. 제가 더 감사합니다!!!☺️

Comment on lines 53 to 55
const { chartInfo, gridInfo } = data;
const { series, options } = chartInfo;
const { chart } = options;
Copy link
Contributor

Choose a reason for hiding this comment

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

그리고 datasets는 type Array로 prop에서 지정해서 상관 없겠지만 destructuring 하기 전 chartInfo, options도 validation 필요할듯 합니다 ! 아니면 에러가 발생할테니까요. destructuring 할때는 이런 주의가 필요합니다.

Suggested change
const { chartInfo, gridInfo } = data;
const { series, options } = chartInfo;
const { chart } = options;
const { chartInfo, gridInfo } = data; // chartInfo, gridInfo 등 값이 없어서 undefined면 아래에서 에러 발생
// const { series, options } = chartInfo // cannot detructure property 에러 발생
// undefined가 아니면 에러는 발생하지 않지만, 원하는 것은 object 이므로 미리 같이 검사를 해줍니다.
if(type(chartInfo) !== 'object') return console.error('...')
const { series, options } = chartInfo;
if(type(options) !== 'object') return console.error('...')
const { chart } = options;

내용이 좀 많죠. 1. type 함수나 toString으로 타입 검사 2. destructuring 하기 전 타입 검사

이렇게 두 가지만 반영하면 될 것 같습니다 !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

보현님 상세한 지적과 조언 너무 감사합니다!

이번 리뷰에 대해서는 destructuring 시 값이 존재하지 않는 경우 발생되는 에러 처리와 함께,
validation이 필요하다고 이해했고 다음과 같이 수정했습니다.

6caea14

1. undefined 에러 핸들링

destructuring된 값이 존재하지 않을 시에는 콘솔에 다음과 같은 에러가 발생합니다.
image

이 에러를 custom (validate)하기 위해,
아래의 참고 자료와 short circuit evaluation을 이용하여
값이 존재하지 않을 때 발생되는 콘솔 에러 대신, undefined로 반환되도록 핸들링해보았습니다.
(예시로 제안해주신 코드와는 다른 형태이긴 합니다)

const data = undefined;  // 임의로 설정
const { chartInfo, gridInfo } = data || {};
const { series, options } = chartInfo || {};
const { chart } = options || {};

console.log(data);  // undefined

short circuit evaluation에 따라,
data, chartInfo, options 등의 값이 undefined일 때 (false)는 {} 빈 객체를 리턴받게 됩니다.
하지만 말 그대로 빈 객체라, destructuring된 값들이 존재하지 않기 때문에(undefined)
결과적으로 undefined가 반환되게 됩니다.

2. type validation

위의 예시 코드에서는 destructuring 전에 먼저 if문에서 type 체크를 해준뒤
조건이 부합하지 않을 시 에러를 발생시키는 방식을 제안해주신 것으로 이해했습니다.

하지만 이전에 datasets의 type 체크를 위한 if문을 이미 구현한 바가 있기 때문에
구문이 중복될 것을 우려하여 별도로 로직을 추가하지는 않았습니다..!
대신 type 케이스에 따라 if문을 나누었고, 에러 메시지를 보다 더 명확하게 제공하도록 수정해보았습니다.

결과 화면

data = undefined;  // 임의로 data를 undefined로 설정
const { chartInfo, gridInfo } = data || {};   // data가 falsy일 때 빈 객체 반환
const { series, options } = chartInfo || {};
const { chart } = options || {};

if (!(this.isType(data) === 'object')) {
    return console.error(
      '[vuetiful-board warn]: Invalid datasets prop: Please check the type or structure of datasets prop. The type of element in datasets must be an object.',
    );
}

설정해둔 error 메시지가 출력된 모습.

image

참고 자료

혹시 잘못 이해하고 있는 부분이 있거나, 틀린 사항이 있다면 지적 부탁드립니다.
잘부탁드립니다!

Copy link
Contributor

Choose a reason for hiding this comment

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

저는 조금 더 로직스러운 면에 집중했던 것 같습니다. 예를들어 chartInfo에 object가 아닌 값이 오거나 undefined이면 그 아래 과정까지 거치지 않게 하자는 생각이었는데 짜신 코드를 보니 가독성을 높이고 chartInfo type 검사도 했네요 더 좋은 것 같습니다 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

감사합니다 보현님!!!

Comment on lines 19 to 37
default: function() {
return [
{
id: this.$uuid.v4(),
chartInfo: {
id: this.$uuid.v4(),
series: [],
options: {
chart: {
type: null,
},
},
},
gridInfo: {
id: this.$uuid.v4(),
},
},
];
},
Copy link
Contributor

Choose a reason for hiding this comment

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

아무 것도 안넣어도 기본 그래프가 그려져서 좋네요

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

감사합니다!

@iamhayoung iamhayoung changed the title [감하영] datasets의 default 값 설정 & datasets의 ID 자동 추가 & datasets type 및 구조 validation [감하영] datasets의 default 값 설정 & datasets의 ID 자동 추가 & datasets type 및 구조 validation 그 외 Aug 31, 2021
@iamhayoung
Copy link
Collaborator Author

iamhayoung commented Aug 31, 2021

@ktseo41

보현님, 라이브러리명 변경 (EasyChartBoard → VuetifulBoard)에 따라,
컴포넌트명과 각종 요소들의 이름을 수정한 점 공유드립니다!

위의 리뷰 반영 사항에 대해서도 확인 부탁드립니다!
잘부탁드립니다.

@iamhayoung
Copy link
Collaborator Author

@ktseo41

보현님! 늦은 시간에 확인 감사합니다!
Nullish Coalescing Operator 관련해서 다시 한번 코멘트를 추가했습니다.
확인 부탁드립니다!

#5 (comment)

@iamhayoung iamhayoung merged commit 10a6024 into develop Sep 1, 2021
@ktseo41 ktseo41 deleted the feature/validateChartProps branch September 1, 2021 15:24
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.

None yet

2 participants