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

AF-3589 Dart 2 migration guide #210

Merged
merged 4 commits into from
Dec 28, 2018
Merged

AF-3589 Dart 2 migration guide #210

merged 4 commits into from
Dec 28, 2018

Conversation

evanweible-wf
Copy link
Contributor

@evanweible-wf evanweible-wf commented Dec 14, 2018

Ultimate problem:

  • Documentation around the migration from Dart 1 to Dart 2 is needed

How it was fixed:

  • Added it!

TODO

Testing suggestions:

  • N/a, docs only

Potential areas of regression:

  • N/a, docs only

FYA: @greglittlefield-wf @aaronlademann-wf @kealjones-wk @corwinsheahan-wf
@sebastianmalysa-wf @robbecker-wf

@aviary3-wk
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.

@codecov-io
Copy link

codecov-io commented Dec 14, 2018

Codecov Report

Merging #210 into master will increase coverage by 0.87%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #210      +/-   ##
==========================================
+ Coverage   95.87%   96.74%   +0.87%     
==========================================
  Files          34       35       +1     
  Lines        1669     2570     +901     
==========================================
+ Hits         1600     2486     +886     
- Misses         69       84      +15

Copy link
Member

@robbecker-wf robbecker-wf left a comment

Choose a reason for hiding this comment

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

Looks great!

Copy link
Contributor

@corwinsheahan-wf corwinsheahan-wf left a comment

Choose a reason for hiding this comment

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

Just some small things. This is really well done and comprehensive!

similar) functionality provided by the Dart 1 transformer, but in the form of a
Dart 2-compatible builder. This process is not as straightforward as it may
seem, unfortunately, because the set of limitations imposed by the builder
pattern is more restrictive than with transformers (for good reason). In
Copy link
Contributor

Choose a reason for hiding this comment

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

#nit We should link to a resource which explains why this is "for good reason" (or briefly explain). It probably won't be immediately obvious why this is the case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had the same thought this morning, I'll try to find a relevant blog post or readme from the dart team

+ UiFactory<FooProps> Foo = $Foo;
```

In this example, `$Foo` would be generated by the builder.
Copy link
Contributor

Choose a reason for hiding this comment

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

#nit

Suggested change
In this example, `$Foo` would be generated by the builder.
In this example, the implementation for `$Foo` is generated by the over_react builder.

expected. Since the public version of the class is generated and includes the
concrete getters and setters _as well_ as everything concrete that was defined
in the `_$` version, you or someone else can simply extend it. No additional
work by the builder required.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
work by the builder required.
work by the builder is required.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be extended by another private version of whatever it is, right? Maybe we can show an example?

Copy link
Contributor

@corwinsheahan-wf corwinsheahan-wf Dec 17, 2018

Choose a reason for hiding this comment

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

Consumers wishing to extend or implement a props/state class will extend from the publicly generated version, in their own private _$ prefixed sub class. Example:

// super_class.dart
@Props()
class _$SuperProps extends UiProps {...}

// super_class.over_react.g.dart (generated)
class SuperProps extends _$SuperProps {
  // concrete implementations of props accessors
}

// sub_class.dart
@Props()
class _$SubProps extends SuperProps {...}

// sub_class.over_react.g.dart (generated)
class SubProps extends _$SubProps {
  // concrete implementations of props accessors
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An example is a good idea, I'll add one.

these mixins that extend the consumer-defined version, the builder will generate
a separate class that implements the consumer-defined mixin. This generated
class will have concrete getters and setters implemented for all of the fields
and any additional concrete methods, getters, and setters will be copied over.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
and any additional concrete methods, getters, and setters will be copied over.
and any additional concrete methods, getters, and setters which are defined in the mixin will be copied over to the generated class.

Use our [`over_react_migrate_to_dart1_and_dart2` codemod script][orcm-dart1-to-dart1-and-dart2]
to update your code to a state that is compatible with both the Dart 1
transformer and the Dart 2 builder. _In this state, you will notice some extra
boilerplate and comments. This will be cleaned up in the second step, but is
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
boilerplate and comments. This will be cleaned up in the second step, but is
boilerplate and comments. This will be cleaned up when the transition to dart 2 is completed and we can pull the plug on dart 1 compatibility, but is

@evanweible-wf evanweible-wf added the dart2 PRs targeting our effort to get to Dart SDK 2.0 compatibility label Dec 17, 2018
2. Use our [`over_react_migrate_from_dart1_to_dart2` codemod script][orcm-dart1-to-dart2]
to automate the migration.

If, however, you do need migrate your `over_react` code from Dart 1 to Dart 2
Copy link
Contributor

Choose a reason for hiding this comment

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

"need to migrate"

Copy link
Contributor

@greglittlefield-wf greglittlefield-wf left a comment

Choose a reason for hiding this comment

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

Looks great; nice job, @evanweible-wf!!

- Initializing component factories
- Generating component factory registrations
- Replacing uninitialized props and state fields with concrete getters and
setters that map to the underlying, untyped `Map`s used by ReactJS.
Copy link
Contributor

Choose a reason for hiding this comment

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

#supernit this is the only line-item with a period

Suggested change
setters that map to the underlying, untyped `Map`s used by ReactJS.
setters that map to the underlying, untyped `Map`s used by ReactJS

@evanweible-wf
Copy link
Contributor Author

@corwinsheahan-wf @greglittlefield-wf @georgelesica-wf comments addressed!

@georgelesica-wf
Copy link
Contributor

+1

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.

Awesome job @evanweible-wf!!!

@@ -255,14 +256,17 @@ props class for that component, use the following utility:

## Migration From Dart 1 to Dart 2

> _The over_react codemod tool is in the process of being open-sourced, but
> until then the links below will not be publicly accessible._
Copy link
Member

Choose a reason for hiding this comment

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

👍

@robbecker-wf
Copy link
Member

+1 LGTM Thanks for adding all the details about the transitional boilerplate!

@robbecker-wf
Copy link
Member

@Workiva/release-management-p

@rm-astro-wf rm-astro-wf merged commit 0617b4f into master Dec 28, 2018
@rm-astro-wf rm-astro-wf deleted the dart2_migration_guide branch December 28, 2018 17:21
@evanweible-wf evanweible-wf added this to the Dart 2 Compat milestone Jan 7, 2019
@evanweible-wf evanweible-wf modified the milestones: Dart 2 Compat, 2.0.0 (Dart 1 & 2 compatibility) Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dart2 PRs targeting our effort to get to Dart SDK 2.0 compatibility Merge Requirements Met RM Ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.