-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Persona: Deprecate primaryText #4811
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
Merged
JasonGore
merged 6 commits into
microsoft:master
from
JasonGore:jagore/deprecate-persona-primaryText
May 10, 2018
Merged
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
2b94e2b
Deprecate Persona's primaryText prop. Replace all usages in codebase.
JasonGore d00bb5d
Add change file and PR link.
JasonGore 014a5ed
Modify guidelines.
JasonGore a6f5a35
Address PR feedback, fix backwards compatibility bug, update Deprecat…
JasonGore 86d1391
Merge branch 'master' into jagore/deprecate-persona-primaryText
JasonGore 1105d8d
Fix deprecation error causing build failure.
JasonGore File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
11 changes: 11 additions & 0 deletions
11
...changes/office-ui-fabric-react/jagore-deprecate-persona-primaryText_2018-05-08-17-37.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| { | ||
| "changes": [ | ||
| { | ||
| "packageName": "office-ui-fabric-react", | ||
| "comment": "Deprecate Persona's primaryText prop and add replacement text prop.", | ||
| "type": "minor" | ||
| } | ||
| ], | ||
| "packageName": "office-ui-fabric-react", | ||
| "email": "jagore@microsoft.com" | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| # Deprecation Guidelines | ||
|
|
||
| This guide outlines recommendations for deprecating API behavior, particularly related to component props. | ||
|
|
||
| An example PR following these steps can be found here: | ||
| **https://github.com/OfficeDev/office-ui-fabric-react/pull/4811** | ||
|
|
||
| ## Deprecation Steps | ||
| 1. Make sure snapshot tests exist covering existing props functionality as a reference check against deprecation changes. | ||
| - Make sure the prop you're changing has representation in the snapshot output. Sometimes this may require getting the component under test into a certain state. | ||
| 1. Keep the tests using deprecated props named in a file with deprecated suffix, such as `Persona.test.tsx` -> `Persona.deprecated.test.tsx`. | ||
| 1. Copy the test's snapshot output (or ensure it's the same if regenerated.) | ||
| 1. Modify existing tests to use new prop. | ||
| - Snapshot output should be identical in most cases, particularly if props naming is the only thing changing. | ||
| 1. Add new property to interface. | ||
| 1. Optionally, temporarily comment out old prop to help find all uses throughout code base and change. Take care that as of writing some uses are not covered by TypeScript as part of build, such as some objects created in tests without type declaration and Screener tests in `apps/vr-tests`. | ||
| - If you use VS Code, there is a task available to help aid called `Typescript (vr-tests) watch` which you can run via `Tasks -> Run Task`. You may have to rebuild occasionally to get the types reflected across packages correctly. | ||
| 1. Move deprecated prop to end of interface, update comments with deprecation description and add @deprecated. | ||
|
|
||
| ```tsx | ||
| /** | ||
| * Primary text to display, usually the name of the person. | ||
| * @deprecated Use 'text' instead. | ||
| */ | ||
| primaryText?: string; | ||
| ``` | ||
|
|
||
| 1. Update component as needed to support both deprecated and new props. | ||
| 1. Make sure old and new tests pass. | ||
| 1. Add call to warnDeprecations in constructor, like: | ||
|
|
||
| ```tsx | ||
| constructor(props: IPersonaCoinProps) { | ||
| super(props); | ||
|
|
||
| // 'primaryText' is deprecated and 'text' should be used instead | ||
| this._warnDeprecations({ 'primaryText': 'text' }); | ||
| } | ||
| ``` | ||
| 1. warnDeprecations will most likely cause deprecated tests to fail, requiring mocking of warnDeprecations. Please make note to clear mock at end of tests as shown below. | ||
|
|
||
| ```tsx | ||
| // Prevent warn deprecations from failing test | ||
| const Utilities = require('@uifabric/utilities/lib/warn'); | ||
|
|
||
| describe('MyTests', () => { | ||
| beforeAll(() => { | ||
| Utilities.warnDeprecations = jest.fn().mockImplementation(() => { /** no impl **/ }); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| Utilities.warnDeprecations.mockClear(); | ||
| }); | ||
|
|
||
| // tests ... | ||
| }); | ||
| ``` | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't be using require here. Amongst other things, Utilities will not be typed as a result.
Recommend:
The only thing I'm a little less sure about is whether there is a need to cleanup the mock after the fact, as the mock seems localized to the tests within the file/module only.
If you do a beforeEach/afterEach reset though, you can then have asserts again warnDeprecation being triggered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think this should also be mockReset and not mockClear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does anybody know of a way to spyOn or mock exported modules from index modules like Utilities.ts? I don't like using the full path show above as most test imports are using
import { setRTL } from '../../Utilities';but I haven't found a workaround yet.