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

fix(NativeSelect): Don't pass unused props to NativeSelect #6663

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

mendrew
Copy link
Contributor

@mendrew mendrew commented Mar 6, 2024

Описание

Исключаем передачу неиспользуемых Select пропсов в NativeSelect.
Вот бы это как-нибудь автоматически ловить.

@mendrew mendrew added this to the v6.0.2 milestone Mar 6, 2024
@mendrew mendrew self-assigned this Mar 6, 2024
@github-actions github-actions bot added the patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча label Mar 6, 2024
Copy link
Contributor

github-actions bot commented Mar 6, 2024

size-limit report 📦

Path Size
JS 352.36 KB (+0.05% 🔺)
JS (gzip) 107.74 KB (+0.05% 🔺)
JS (brotli) 89.12 KB (+0.05% 🔺)
JS import Div (tree shaking) 1.43 KB (0%)
CSS 257.99 KB (0%)
CSS (gzip) 33.81 KB (0%)
CSS (brotli) 27.44 KB (0%)

Copy link

codesandbox-ci bot commented Mar 6, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Contributor

github-actions bot commented Mar 6, 2024

e2e tests

Playwright Report

Copy link
Contributor

github-actions bot commented Mar 6, 2024

👀 Docs deployed

Commit c9b2e2d

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.21%. Comparing base (e140ea9) to head (c9b2e2d).
Report is 9 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6663   +/-   ##
=======================================
  Coverage   82.21%   82.21%           
=======================================
  Files         331      331           
  Lines       10250    10250           
  Branches     3439     3439           
=======================================
  Hits         8427     8427           
  Misses       1823     1823           
Flag Coverage Δ
unittests 82.21% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mendrew mendrew marked this pull request as ready for review March 6, 2024 13:31
@mendrew mendrew requested a review from a team as a code owner March 6, 2024 13:31
@mendrew mendrew merged commit 39192cc into master Mar 7, 2024
49 checks passed
@mendrew mendrew deleted the mendrew/remove-unused-props-from-native-select branch March 7, 2024 10:04
vkcom-publisher pushed a commit that referenced this pull request Mar 7, 2024
Исключаем передачу неиспользуемых Select пропсов в NativeSelect.
mendrew added a commit that referenced this pull request Mar 7, 2024
Исключаем передачу неиспользуемых Select пропсов в NativeSelect.
@mendrew mendrew mentioned this pull request Mar 7, 2024
@inomdzhon
Copy link
Contributor

inomdzhon commented Mar 7, 2024

Вот бы это как-нибудь автоматически ловить.

@mendrew нужен вот такой тип

  const nativeProps: {
    [K in keyof typeof restProps]: K extends keyof NativeSelectProps
      ? NativeSelectProps[K]
      : never;
  } = restProps;
image

выявил, что ещё after с mode попадает в NativeSelect

@mendrew
Copy link
Contributor Author

mendrew commented Mar 7, 2024

Вот бы это как-нибудь автоматически ловить.

@mendrew нужен вот такой тип

  const nativeProps: {
    [K in keyof typeof restProps]?: K extends keyof NativeSelectProps
      ? NativeSelectProps[K]
      : never;
  } = restProps;

выявил, что ещё after с mode попадает в NativeSelect

🔥 TS master.
Я как-то пробовал основательно к этому подойти, но всё что-то в типах мешало.

@inomdzhon
Copy link
Contributor

Тут можно через TS валидировать

Вот бы это как-нибудь автоматически ловить.

@mendrew нужен вот такой тип

  const nativeProps: {
    [K in keyof typeof restProps]?: K extends keyof NativeSelectProps
      ? NativeSelectProps[K]
      : never;
  } = restProps;

выявил, что ещё after с mode попадает в NativeSelect

🔥 TS master. Я как-то пробовал основательно к этому подойти, но всё что-то в типах мешало.

Да я сам полчаса бился, чтобы к этому типу придти 😅 Сначала через всякие Omit'ы и Exclude'ы пытался добиться должного поведения

@mendrew
Copy link
Contributor Author

mendrew commented Mar 7, 2024

Да я сам полчаса бился, чтобы к этому типу придти 😅 Сначала через всякие Omit'ы и Exclude'ы пытался добиться должного поведения

Это правда потрясающе!!! А я уже не раз бился, но всё не получалось!
Я знаю у нас ещё есть места где это нужно. Пойду искать.

@inomdzhon
Copy link
Contributor

UPD в комменте #6663 (comment) поправил типы, забыл удалить ? – он лишний (вроде)

@mendrew
Copy link
Contributor Author

mendrew commented Mar 7, 2024

UPD в комменте #6663 (comment) поправил типы, забыл удалить ? – он лишний (вроде)

Да, лишний. Тоже это заметил. Спасибо, что поправил)

mendrew added a commit that referenced this pull request Mar 12, 2024
В тех местах, где при определённых условиях под капотом используется нативный компонент, мы, бывает, забываем исключать новые свойства необходимые только в кастомных компонентах. В итоге неспользуемые свойства оказываются в DOM. (#6663, #6386).

Добавил тип, как посоветовал @inomdzhon в #6663 (comment), чтобы при добавлении нового свойства линтер ругался если для нативного компонента это свойство не нужно.
vkcom-publisher pushed a commit that referenced this pull request Mar 12, 2024
В тех местах, где при определённых условиях под капотом используется нативный компонент, мы, бывает, забываем исключать новые свойства необходимые только в кастомных компонентах. В итоге неспользуемые свойства оказываются в DOM. (#6663, #6386).

Добавил тип, как посоветовал @inomdzhon в #6663 (comment), чтобы при добавлении нового свойства линтер ругался если для нативного компонента это свойство не нужно.
@vkcom-publisher
Copy link
Contributor

v6.0.2 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmp:native-select patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants