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

FED-3160 Improve null safety docs and messages for wrapper components and connect #949

Merged
merged 4 commits into from
Sep 23, 2024

Conversation

greglittlefield-wf
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf commented Sep 20, 2024

Motivation

The over_react null safety migration guide currently doesn't mention that:

Also, the messages in the analyzer plugin required props lint and runtime prop validation aren't very helpful when hitting these cases.

We should document these issues properly, and make it easier for consumers to find the information they need to resolve these issues.

Changes

  • Add docs around connect and late props
  • Add info to migration guide for connect and wrappers
  • Required props lint: improve docs, mention edge-cases, add links
  • Make missing required props runtime error more helpful by including more context and a link to the docs

Release Notes

Review

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

Please review:

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:
      • Rendered markdown docs look good and there are no broken links
      • Analyzer plugin info looks good in playground and references docs
        • This is what it looks like for me:
          • Screenshot 2024-09-20 at 10 51 08 AM
        The actual documentation link points to here https://workiva.github.io/over_react/analyzer_plugin/lints/over_react_late_required_prop.html and is currently broken because we haven't redeployed that lints docs site in a while, but we can do so after this merges
      • The runtime error message when a required prop is missing is helpful and has a non-broken link
    • 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 Frontend Frameworks Design 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

@aviary-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.

@greglittlefield-wf greglittlefield-wf changed the title Improve null safety docs and messages for wrapper components and connect FED-3160 Improve null safety docs and messages for wrapper components and connect Sep 20, 2024
@greglittlefield-wf greglittlefield-wf marked this pull request as ready for review September 20, 2024 17:53

#### connect

We'll be adding a codemod to help migrate the `connect` case: https://github.com/Workiva/over_react_codemod/issues/295
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a mention here of what the migration looks like - like they could add the disable validation line themselves too, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

just to make it clear that they don't need to wait for the codemod

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured that was covered in the above section, which links to the disabling validation docs section that contains the connect cases, but I can definitely clarify that here and maybe link more specifically

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense! I could go either way, but reading it initially, having that section start with "We'll be adding a codemod" makes it sound like they have to wait for us to do that 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh for sure, and I think it should be clarified; I was more over-explaining why I initially wrote it that way, and that there was technically a link.

Addressed in 64c00a0; let me know what you think!

Copy link
Contributor

Choose a reason for hiding this comment

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

I love it! Thank you!

doc/null_safety/null_safe_migration.md Outdated Show resolved Hide resolved
Copy link
Contributor

@sydneyjodon-wk sydneyjodon-wk left a comment

Choose a reason for hiding this comment

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

+10

@greglittlefield-wf
Copy link
Contributor Author

@Workiva/release-management-p

Copy link

@rmconsole-wf rmconsole-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 from RM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants