fluentui icon indicating copy to clipboard operation
fluentui copied to clipboard

[Feature]: Add conformance test to ensure context exports are added to the package

Open sopranopillow opened this issue 1 year ago • 3 comments

Library

React Components / v9 (@fluentui/react-components)

Describe the feature that you would like added

After talking with some partners it is apparent that when the partner is recomposing components and the component's API later adds a context (adding usecontextvalues) to the component definition, it blocks recomposition (more important bumping versions of fluent) by not exporting these values. We are humans so this mistake can happen, but this proves to be a bad DX for partners.

Have you discussed this feature with our team

No

Additional context

Related: #31328

Validations

  • [X] Check that there isn't already an issue that request the same feature to avoid creating a duplicate.

Priority

High

sopranopillow avatar May 09 '24 18:05 sopranopillow

This is rather tricky IMO, it should be generalized to "package exports public api surface" rather than "context only - which is not present in every pacakge", but in order to determine that we need to ask a question, what is public API surface and how can we determine that ?

approach that I can think of right awau is to have a 2 parts rule:

  • explicitly annotate every token/function via @public JSDoc annotation which is supposed to be exported from package public API, and check index.ts if it contains that token
  • general rule of V9 framework public API exports (render,style,state hooks) and based on that check index.ts if these symbols are exported. while we can leverage this one based on naming, that seems to be brittle long term so again probably some kind of metadata annotation via JSDoc should be implemented

Sidenote: this should not be part of conformance test as the overall solution should not be expanded rather ported to custom workspace eslint rules that provides significantly better DX and is much more performant, obviously some patterns can be challenging/impossible to implemented on Eslint side ( probably my 1st rule which would need to parse all sources, gather symbols containing @public JSDOC and compare with export tokens within index.ts ) .

WDYT ?

Hotell avatar May 10 '24 08:05 Hotell

Yeah I think the 1st options sounds the best path. iirc we were going to do this before, but can't remember why we ended up not using those tags. While it does seem like a drag to add this, it would greatly improve DX for partners that use our components by recomposing. Also like you pointed out, the 2nd option may bring issues along the road so I think we should stay away from that kind of path.

Thanks for the side note :) I always immediately think conformance tests, but maybe I should say conformance rules(?) Just want o make sure all components follow this set of rules.

sopranopillow avatar May 10 '24 20:05 sopranopillow

Would like to add a requirement here that XXXContext it self should not be exported but rather:

  1. XXXContextProvider component
  2. useXXXContext hook

This reduces the public API surface and avoids users from abusing context by using React.useContext calls directly

ling1726 avatar Jun 25 '24 15:06 ling1726

This issue has not had activity for over 180 days! We're adding Soft close label and will close it soon for house-keeping purposes. Still require assistance? Please add comment - "keep open".