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

Implement FeatureProvider #1309

Merged
merged 16 commits into from
Apr 26, 2023

Conversation

sungik-choi
Copy link
Contributor

@sungik-choi sungik-choi commented Apr 24, 2023

Self Checklist

  • I wrote a PR title in English.
  • I added an appropriate label to the PR.
  • I wrote a commit message in English.
  • I wrote a commit message according to the Conventional Commits specification.
  • I added the appropriate changeset for the changes.
  • [Component] I wrote a unit test about the implementation.
  • [Component] I wrote a storybook document about the implementation.
  • [Component] I tested the implementation in various browsers.
    • Windows: Chrome, Edge, (Optional) Firefox
    • macOS: Chrome, Edge, Safari, (Optional) Firefox
  • [New Component] I added my username to the correct directory in the CODEOWNERS file.

Related Issue

Fixes #1301

Summary

  • 전역으로 피쳐를 On/Off 할 수 있는 FeatureProvider 를 구현합니다.
  • 현재 피쳐는 smooth corners 만 존재합니다.

TODO

  • add changeset
  • FeatureFlagContext 를 consume하는 SmoothCornersBox 재사용 가능한 컴포넌트 구현
    • 현재 이 PR 그대로 적용 시, Avatar 초기 렌더링 시 smooth corners가 적용되지 않아 보이는 경우가 있음. React 외부의 상태를 바라보고 있기 때문이며, 추후 위 컴포넌트 구현으로 해결될 것으로 기대합니다

Details

초기엔 BezierProviderfeatures prop을 추가하여 기존 프로바이더에 녹여내는 방식을 고민해봤습니다. 하지만 BezierProvider 가 현재 채널 데스크 어플리케이션에서 컨텍스트를 중첩하여 테마를 오버라이드하는 방식으로 사용되고 있기 떄문에, 테마와는 큰 관련이 없는 피쳐 플래그에 대한 책임을 BezierProvider 에 부여하는 건 적절치 못한 방법이라고 판단했습니다.

피쳐 플래그에 대한 책임만을 담당하는 별도의 컨텍스트를 두는 방식이 기존 사용처에서 변경은 있으나, tree shaking + 이후 확장성을 고려해봤을 때 적절한 솔루션이라고 판단해 이 방향으로 구현했습니다.

  • 해당 컨텍스트에 Feature 클래스의 인스턴스를 전달하면 해당하는 피쳐를 활성화하는 방식으로 동작합니다.
  • SmoothCorners 의 경우, 컨텍스트를 중첩하더라도 스크립트를 계속 등록할 필요는 없기 때문에 싱글턴 + 내부에 activated 플래그를 두고 조건부 로직을 추가하는 방식으로 초기 스크립트 1회 등록 이후에 추가 오버헤드가 발생하지 않도록 했습니다.
  • 디테일한 내용은 셀프 코드 리뷰로 남깁니다.

Breaking change or not (Yes/No)

Yes

WIP

References

@sungik-choi sungik-choi added enhancement Issues or PR related to making existing features better rfc labels Apr 24, 2023
@sungik-choi sungik-choi self-assigned this Apr 24, 2023
@changeset-bot
Copy link

changeset-bot bot commented Apr 24, 2023

🦋 Changeset detected

Latest commit: dd06191

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@channel.io/bezier-react Minor
bezier-figma-plugin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@sungik-choi sungik-choi changed the title Implement FeatureProvider [rfc] Implement FeatureProvider Apr 24, 2023
@sungik-choi sungik-choi changed the title [rfc] Implement FeatureProvider [RFC] Implement FeatureProvider Apr 24, 2023
@codecov
Copy link

codecov bot commented Apr 24, 2023

Codecov Report

Patch coverage: 36.36% and project coverage change: -0.50 ⚠️

Comparison is base (06d2a5f) 78.18% compared to head (dd06191) 77.69%.

Additional details and impacted files
@@             Coverage Diff             @@
##           next-v1    #1309      +/-   ##
===========================================
- Coverage    78.18%   77.69%   -0.50%     
===========================================
  Files          294      300       +6     
  Lines         3796     3842      +46     
  Branches       838      842       +4     
===========================================
+ Hits          2968     2985      +17     
- Misses         545      571      +26     
- Partials       283      286       +3     
Impacted Files Coverage Δ
.../Avatars/CheckableAvatar/CheckableAvatar.styled.ts 0.00% <0.00%> (ø)
...ages/bezier-react/src/providers/BezierProvider.tsx 100.00% <ø> (ø)
...ages/bezier-react/src/features/FeatureProvider.tsx 20.00% <20.00%> (ø)
packages/bezier-react/src/foundation/Mixins.ts 83.33% <25.00%> (-5.96%) ⬇️
...tures/SmoothCornersFeature/SmoothCornersFeature.ts 28.57% <28.57%> (ø)
packages/bezier-react/src/features/index.ts 60.00% <60.00%> (ø)
...act/src/components/Avatars/Avatar/Avatar.styled.ts 77.77% <66.66%> (-3.71%) ⬇️
packages/bezier-react/src/features/Feature.ts 80.00% <80.00%> (ø)
...r-react/src/features/SmoothCornersFeature/index.ts 100.00% <100.00%> (ø)
...atures/SmoothCornersFeature/smoothCornersScript.ts 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link
Contributor

github-actions bot commented Apr 24, 2023

Chromatic Report

🚀 Congratulations! Your build was successful!

@@ -29,7 +29,7 @@ interface StatusWrapperProps extends Pick<AvatarProps, 'showBorder' | 'size'> {}

function calcStatusGap({ showBorder, size }: StatusWrapperProps) {
let gap = (size >= AvatarSize.Size72 ? 4 : -2)
if (showBorder && enableSmoothCorners.current) {
if (showBorder && SmoothCornersFeature.activated) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

현재 스타일링 방식으로 React 컨텍스트(피쳐 플래그)에 접근하여 조건을 판단하는 방식은 당장 적용하긴 어렵습니다. 이 PR의 컨텍스트를 간결하게 유지하기 위해서 activated 필드를 퍼블릭으로 열어주고, 추후 컨텍스트에 접근하여 Smooth Corners 기능을 활성화하는 SmoothCornersBox 같은 이름의 재사용 가능한 컴포넌트를 별도로 구현할 예정입니다.

)

// @ts-ignore
const promise = CSS.paintWorklet.addModule(workletURL) as Promise<void>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +105 to +106
${SmoothCornersFeature.activated && css`
@supports (background: paint(smooth-corners)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

기존엔 BezierProvider 에서 무조건 Smooth corners 스크립트를 활성화했기 때문에 문제가 없는 코드였습니다. 하지만 이번에 조건부로 활성화할 수 있게 변경되면서 테스트해보았더니, background: paint(smooth-corners) 판별 로직이 실제 모듈이 worklet에 추가되었는지 여부를 판별하지 않고 활성화되는 걸 확인했습니다(mac Chrome 112.0.5615.49)

오동작을 방지하기 위하여 SmoothCornersFeature.activated 플래그를 우선 명시적으로 추가합니다. 위 코멘트에 설명했듯 이 플래그에 직접 접근하는 방식은 컨텍스트의 피쳐 플래그에 접근하는 방식으로 변경할 예정입니다.

@sungik-choi sungik-choi requested review from Dogdriip and dinohan April 24, 2023 09:06
@sungik-choi sungik-choi mentioned this pull request Apr 24, 2023
9 tasks
* 🚨 This is an experimental feature! It may not be suitable for use in production.
*
* Instead of CSS border-radius, Use *Superellipse* masking using the CSS Houdini API.
* When enabled, the feature will be applied to components with the `smoothCorners` property set to `true`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SmoothCornersBox 컴포넌트 분리 후, Avatar 컴포넌트에 기본값을 true 로 하는 옵셔널 smoothCorners 속성을 추가할 예정입니다.

@sungik-choi sungik-choi changed the title [RFC] Implement FeatureProvider Implement FeatureProvider Apr 24, 2023
...flags.reduce((acc, cur) => ({ ...acc, ...cur }), {}),
}))
})
}, [features])
Copy link
Member

Choose a reason for hiding this comment

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

사용처에서 잘못 사용하면 매 렌더링마다 activateFeatures effect가 트리거될 수 있겠네요.
배열의 reference가 렌더링때마다 바뀔 것 같아요.

Copy link
Contributor Author

@sungik-choi sungik-choi Apr 24, 2023

Choose a reason for hiding this comment

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

넵 그래서 최대한 루트에 위치시키도록 가이드하고, 되도록이면 추가로 사용처에서 메모이제이션해주는 게 좋을 거 같습니다.

@dinohan dinohan self-requested a review April 25, 2023 11:35
Copy link
Contributor

@Dogdriip Dogdriip left a comment

Choose a reason for hiding this comment

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

현재는 FeatureProvider 내 effect 로직이 다소 복잡하게 구현되어 있는 것 같은데, #1301 close가 우선인 것으로 생각하고 나중에 리팩토링해보면 좋을 것 같습니다 👍

(+ codeowner 추가 부탁드립니다!)

@sungik-choi sungik-choi merged commit 04e0520 into channel-io:next-v1 Apr 26, 2023
@sungik-choi sungik-choi deleted the feat/feature-provider branch April 26, 2023 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues or PR related to making existing features better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run EnableCSSHoudini conditionally
3 participants