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.

Sign up for GitHub

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

Jump to bottom

CPLAT-6938 Add flag to upgrade abstract components #46

Merged
merged 33 commits into from
Sep 16, 2019
Merged

CPLAT-6938 Add flag to upgrade abstract components #46

merged 33 commits into from
Sep 16, 2019

Conversation

sydneyjodon-wk
Copy link
Contributor

@sydneyjodon-wk sydneyjodon-wk commented Sep 9, 2019 •
edited
Loading

Reliant on CPLAT-6936 (#42)

Because this PR uses the test functions created in #42, the base of this branch is CPLAT-6936-no-partial-upgrades-flag.

Motivation

If a component is upgraded to Component2, any components that extend from it could potentially break. Within a library, this is fine, but it's problematic if the component is exported.

For an easier incremental upgrade process, we should an option to have our codemod only upgrade components that can't be extended from.

That way, we can run and commit these partial upgrades separately, which will make rolling them back easier if it's determined that they cannot be upgraded without causing any breakages.

Changes

  • Change existing codemod to only upgrade non-abstract components by default.
  • Add a flag --upgrade-abstract-components to Component2 codemod executable that will allow the codemod to upgrade abstract components.
    • Pass in as a parameter to suggestors telling them to upgrade abstract components
    • When abstract components are upgraded, add a fixme comment before the class declaration that warns the consumer that this is a breaking change if the component class is exported.
  • Create a utility function that determines whether a component can be extended from, that can be shared across multiple suggestors. Criteria:
    • A component class can be extended from if one of the following is true:
      • abstract keyword on component class
      • Generic parameters on component class
      • @AbstractProps in the same file
  • Write test coverage for each suggestor using the flag

Release Notes

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review:
@kealjones-wk @aaronlademann-wf @greglittlefield-wf @joebingham-wk

QA Checklist

  • Tests were updated and provide good coverage of the changeset and other affected code
  • Manual testing was performed if needed
    • Steps from PR author:
      • CI passes
      • Run the Component2 codemod on web_skin_dart with the --upgrade-abstract-components flag and verify that the codemod updates abstract components based on the above criteria.
        • pub global run over_react_codemod:component2_upgrade --upgrade-abstract-components
    • Anything falling under manual testing criteria outlined in CONTRIBUTING.md

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

  • A Client Platform member has reviewed these changes
  • There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
  • For release PRs - Version metadata in Rosie comment is correct

@sydneyjodon-wk
Add --upgrade-abstract-components flag
81426c4
@sydneyjodon-wk
Format
8fa0c41
@sydneyjodon-wk
Update to fix warnings
b9930d1
@sydneyjodon-wk
Merge branch 'CPLAT-5212_component_2_migration/class_annotation' into…
1458d04 
… CPLAT-6938-upgrade-abstract-components-flag
@sydneyjodon-wk
Merge branch 'CPLAT-6936-no-partial-upgrades-flag' into CPLAT-6938-up…
dc222a0 
…grade-abstract-components-flag

# Conflicts:
#	lib/src/component2_suggestors/class_name_and_annotation_migrator.dart
#	lib/src/component2_suggestors/componentdidupdate_migrator.dart
#	lib/src/component2_suggestors/componentwillmount_migrator.dart
#	lib/src/component2_suggestors/copyunconsumeddomprops_migrator.dart
#	lib/src/component2_suggestors/deprecated_lifecycle_suggestor.dart
#	lib/src/component2_suggestors/setstate_updater.dart
#	lib/src/executables/component2_upgrade.dart
@sydneyjodon-wk
Add --upgrade-abstract-components flag
a6dcb5f
@sydneyjodon-wk
Add canBeExtendedFrom utility function
36c0eaa
@sydneyjodon-wk
Add canBeExtendedFrom check to suggestors
74e8d97
@sydneyjodon-wk
Update tests to be more generic
4907082
@sydneyjodon-wk
Update the doc comment
a8537bb
@sydneyjodon-wk
Update tests with --upgrade-abstract-components flag
4698d11
@sydneyjodon-wk
Add test cases to componentDidUpdate tests
ca47254
@sydneyjodon-wk
Format
45be71b
@sydneyjodon-wk
Add test cases to componentWillMount tests
d46b024
@sydneyjodon-wk
Add test cases to copyUnconsumedDomProps tests
29a5b5e
@sydneyjodon-wk
Add test cases to deprecated lifecycle tests
cc23e0a
@sydneyjodon-wk
Add test cases to setState tests
6e14dc6
@sydneyjodon-wk
Add test cases to class name and annotation tests
501feba
@sydneyjodon-wk
Merge branch 'CPLAT-6936-no-partial-upgrades-flag' into CPLAT-6938-up…
8895c8f 
…grade-abstract-components-flag
@sydneyjodon-wk
Update utility to check for generic parameters
02aef9d
@sydneyjodon-wk
Update class name tests
769beb1
@sydneyjodon-wk
Update utility tests with generic parameter testcase
3ed8366
@sydneyjodon-wk
Update tests
17cd6f9
@sydneyjodon-wk
Add fixme comments for upgraded abstract components
a4842b0
@sydneyjodon-wk
Update tests
6efeed1
@aviary2-wf
Copy link

Security Insights

No security relevant content was detected by automated scans.

Action Items

  • Review PR for security impact; comment "security review required" if needed or unsure
  • Verify aviary.yaml coverage of security relevant code

Questions or Comments? Reach out on Slack: #support-infosec.

@sydneyjodon-wk sydneyjodon-wk changed the base branch from CPLAT-6936-no-partial-upgrades-flag to master September 9, 2019 21:58
@sydneyjodon-wk sydneyjodon-wk changed the base branch from master to CPLAT-6936-no-partial-upgrades-flag September 11, 2019 16:11
@sydneyjodon-wk sydneyjodon-wk changed the base branch from CPLAT-6936-no-partial-upgrades-flag to master September 11, 2019 16:11
kealjones-wk
kealjones-wk requested changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@kealjones-wk kealjones-wk left a comment

Choose a reason for hiding this comment

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

Woops

lib/src/component2_suggestors/component2_utilities.dart Outdated Show resolved Hide resolved
test/component2_suggestors/class_name_and_annotation_migrator_test.dart Outdated
@@ -148,7 +167,7 @@ void classNameAndAnnotationTests({bool allowPartialUpgrades}) {
expectedPatchCount: allowPartialUpgrades ? 2 : 0,
input: '''
@Component()
class FooComponent extends UiComponent<FooProps> {
class FooComponent extends UiComponent {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am assuming you removed all these generics aka <FooProps> because you thought that meant this class was abstract. Thats my bad for not fully explaining. Having the generics aka <FooProps> on the class that this class extends from is fine and doesn't make it "abstract". Having a generic on the class name itself would make it extendable/abstract...
so if it looks like this:

Suggested change
class FooComponent extends UiComponent {
class FooComponent<BarProps> extends UiComponent<FooProps> {

In the above the <BarProps> is what makes this component "extendable"/"abstract"

test/component2_suggestors/class_name_and_annotation_migrator_test.dart Outdated Show resolved Hide resolved
test/component2_suggestors/class_name_and_annotation_migrator_test.dart Outdated Show resolved Hide resolved
test/component2_suggestors/class_name_and_annotation_migrator_test.dart Outdated Show resolved Hide resolved
test/component2_suggestors/class_name_and_annotation_migrator_test.dart Outdated Show resolved Hide resolved
test/component2_suggestors/class_name_and_annotation_migrator_test.dart Outdated Show resolved Hide resolved
test/component2_suggestors/class_name_and_annotation_migrator_test.dart Outdated Show resolved Hide resolved
test/component2_suggestors/copyunconsumeddomprops_migrator_test.dart Outdated Show resolved Hide resolved
kealjones-wk
kealjones-wk approved these changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@kealjones-wk kealjones-wk left a comment

Choose a reason for hiding this comment

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

+1

aaronlademann-wf
aaronlademann-wf approved these changes Sep 16, 2019
View reviewed changes
Copy link
Contributor

@aaronlademann-wf aaronlademann-wf left a comment

Choose a reason for hiding this comment

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

+1

@Workiva/release-management-pp

@rmconsole2-wf rmconsole2-wf merged commit c2a1dd0 into master Sep 16, 2019
@rmconsole2-wf rmconsole2-wf deleted the CPLAT-6938-upgrade-abstract-components-flag branch September 16, 2019 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kealjones-wk kealjones-wk kealjones-wk approved these changes

@aaronlademann-wf aaronlademann-wf aaronlademann-wf approved these changes

Assignees
No one assigned
Labels
Merge Requirements Met RM Ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@sydneyjodon-wk @aviary2-wf @aaronlademann-wf @kealjones-wk @rmconsole-wf @rmconsole2-wf @greglittlefield-wf