Skip to content

IconNames: Deprecate due to const enum usage#8005

Merged
msft-github-bot merged 9 commits into
microsoft:masterfrom
JasonGore:jg/7110-deprecate-iconNames
Feb 15, 2019
Merged

IconNames: Deprecate due to const enum usage#8005
msft-github-bot merged 9 commits into
microsoft:masterfrom
JasonGore:jg/7110-deprecate-iconNames

Conversation

@JasonGore

@JasonGore JasonGore commented Feb 14, 2019

Copy link
Copy Markdown
Contributor

Pull request checklist

Description of changes

Per #7110 deprecate IconNames due to its usage of const enum.

Microsoft Reviewers: Open in CodeFlow

@msft-github-bot

Copy link
Copy Markdown
Contributor

Hello @JasonGore!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull-requests of this repository that have been opened for at least 8 hours, a condition that is not currently met. No worries though, I will be back when the time is right! 😉

@size-auditor

size-auditor Bot commented Feb 14, 2019

Copy link
Copy Markdown

Size Auditor did not detect a change in bundle size for any component!

Comment thread packages/icons/src/IconNames.ts Outdated
}

/**
* @deprecate IconNames is deprecated.

@dzearing dzearing Feb 15, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this deprecated? I don't think the type IconNamesInput would need to be deprecated.

Also, everything in the @uifabric/icons package is generated by the icon subsetting tool. You can talk with @Jahnp here, but we should change the tool first if we want to make any changes.

@JasonGore JasonGore Feb 15, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is dependent on an deprecated object, and could also result in the issue people are seeing in #7110, so that is why I also marked this deprecated. Does that make sense?

I will talk to Peter about the tool, thanks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

More simply put maybe I should have just said it's a deprecated type, since it's keying off a deprecated object. 😄

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe @ThomasMichon added this. A better approach would be to keep the type, but just generate the type. adding this was a cheap way to get some type safety.

also, the tag should be @deprecated, not @deprecate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I removed this change before the PR was merged.

@KevinTCoughlin

Copy link
Copy Markdown
Member

Last I checked, SPFx was using string union type, but will double check. cc: @patmill

@JasonGore JasonGore requested a review from Jahnp February 15, 2019 16:58
@JasonGore

Copy link
Copy Markdown
Contributor Author

Spoke with @Jahnp and we may decide to close this PR and make another one from the upstream tool changes. I'll mark "Do not merge" for now and close/update within the next day or so.

@Jahnp

Jahnp commented Feb 15, 2019

Copy link
Copy Markdown
Member

@JasonGore and I chatted some more offline. This works great for now--we'll go ahead and deprecate the IconNames enum and plan to remove it for 7.0. We'll keep IconNamesInput, and in 7.0 replace it with a plan vs inferred string union.

@KevinTCoughlin KevinTCoughlin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Spoke with SPFx offline, signing-off

@msft-github-bot msft-github-bot merged commit 445121a into microsoft:master Feb 15, 2019
@msft-github-bot

Copy link
Copy Markdown
Contributor

🎉@uifabric/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@msft-github-bot

Copy link
Copy Markdown
Contributor

🎉[email protected] has been released which incorporates this pull request.:tada:

Handy links:

@BTC-Bradley

Copy link
Copy Markdown

we'll go ahead and deprecate the IconNames enum and plan to remove it for 7.0.

@Jahnp, is there a road-map w/ ETA for the 7.0 release?

@Jahnp

Jahnp commented Feb 20, 2019

Copy link
Copy Markdown
Member

@BTC-Bradley project planning for 7.0 is early but actively being worked on. We're hoping to have it out within the next few months, but that's pending some investigations. You can track progress here (project is being built up & managed by @jdhuntington): https://github.com/OfficeDev/office-ui-fabric-react/projects/29

@microsoft microsoft locked as resolved and limited conversation to collaborators Aug 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

7 participants