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

feat: use @lit/task to schedule async data fetching #2912

Merged
merged 29 commits into from
Jan 22, 2024
Merged

Conversation

gavinbarron
Copy link
Member

@gavinbarron gavinbarron commented Dec 13, 2023

Closes #2861

PR Type

  • Bugfix
  • Feature

Description of the changes

This PR introduces a new approach to loading component state via async calls.
The approach provided in the MgtBaseComponent and MgtTemplatedComponent relied on manually calling the requestUpdate method and exposing a requestStateUpdate method to manually load external state via an http call.
This had a side effect of scheduling updates during an update cycle which is inefficient and results in a warning emitted to the console when running in developer mode.

The new approach introduces two replacement base classes MgtBaseTaskComponent and MgtTemplatedTaskComponent which should be used to replace their respective predecessors.

@lit/task is used to wrap up the existing loadState method in each component, this is supplied by MgtBaseTaskComponent. Implementing classes must implement:

  • args: () => unknown[] instance method. The array returned should contain all of the reactive properties on the object which when changed will trigger loadState method to be called. This array should contain the providerState property from the base class.
  • renderContent: () => TemplateResult the existing render method should be renamed to this. The implementation of the render method is provided by each of the base classes and provides default implementations for rendering while the task execution is in progress and if an error occurs. You may of course provide component specific overrides for any of these methods as needed. 
  • @property decorators on fields with custom accessor methods should be moved to decorate the setter method rather than the getter as was done previously. If a field is calling requestStateUpdate then it should be present in the array returned from the args method
  • JSDoc comments MUST be left on the getters to ensure that code generation for the React wrapper library is left intact.

The existing base classes will be marked as @deprected when the migration is complete and may be removed in a future version.

Testing of these changes needs to be done locally, The react-contoso application is a good test bed, however it's important to try navigating away and back to a page with the component being tested as sometimes the warning doesn't occur on a full page reload.

As this is a large change I'd recommend reviewing the changes by looking at each commit as a change to help with chunking this. after the initial commits getting the pattern set I've kept the changes in a single commit to cover a single component or set of related components.

PR checklist

  • Project builds (yarn build) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)
  • All public APIs (classes, methods, etc) have been documented following the jsdoc syntax
  • Stories have been added and existing stories have been tested
  • Added appropriate documentation. Docs PR:
  • License header has been added to all new source files (yarn setLicense)
  • Contains NO breaking changes

Other information

Converted components:

  • mgt-agenda
  • mgt-contact
  • mgt-file
  • mgt-file-list
  • mgt-get
  • mgt-login
  • mgt-messages
  • mgt-organization
  • mgt-people
  • mgt-people-picker
  • mgt-person
  • mgt-person-card
  • mgt-picker
  • mgt-planner
  • mgt-profile
  • mgt-search-box
  • mgt-search-results
  • mgt-tasks-base
  • mgt-taxonomy-picker
  • mgt-teams-channel-picker
  • mgt-theme-toggle
  • mgt-todo
  • mgt-arrow-options
  • mgt-dot-options
  • mgt-flyout
  • mgt-spinner

Copy link

🚀 New react-contoso sample application deployed here

1 similar comment
Copy link

🚀 New react-contoso sample application deployed here

Copy link

📖 The updated storybook is available here

1 similar comment
Copy link

📖 The updated storybook is available here

Copy link

🚀 New react-contoso sample application deployed here

Copy link

📖 The updated storybook is available here

Copy link

🚀 New react-contoso sample application deployed here

Copy link

📖 The updated storybook is available here

Copy link

🚀 New react-contoso sample application deployed here

Copy link

📖 The updated storybook is available here

Copy link

🚀 New react-contoso sample application deployed here

Copy link

📖 The updated storybook is available here

Copy link

📖 The updated storybook is available here

Copy link

🚀 New react-contoso sample application deployed here

Copy link

📖 The updated storybook is available here

Copy link

🚀 New react-contoso sample application deployed here

Copy link

github-actions bot commented Jan 2, 2024

🚀 New react-contoso sample application deployed here

Copy link

github-actions bot commented Jan 2, 2024

📖 The updated storybook is available here

@sebastienlevert
Copy link
Contributor

After review, when the initial load of the Teams channel picker happens, we should have a loading state withing the flyout and not in place of the input.

Copy link

github-actions bot commented Jan 3, 2024

🚀 New react-contoso sample application deployed here

Copy link

github-actions bot commented Jan 3, 2024

📖 The updated storybook is available here

Copy link

github-actions bot commented Jan 3, 2024

🚀 New react-contoso sample application deployed here

Copy link

github-actions bot commented Jan 3, 2024

📖 The updated storybook is available here

@gavinbarron gavinbarron linked an issue Jan 5, 2024 that may be closed by this pull request
Copy link

🚀 New react-contoso sample application deployed here

Copy link

Code Coverage

Package Line Rate Branch Rate Complexity Health
mgt-components.src.components 86% 100% 0
mgt-components.src.components.mgt-contact 63% 100% 0
mgt-components.src.components.mgt-file 60% 100% 0
mgt-components.src.components.mgt-file-list 46% 100% 0
mgt-components.src.components.mgt-file-list.mgt-file-upload 49% 88% 0
mgt-components.src.components.mgt-get 23% 100% 0
mgt-components.src.components.mgt-messages 68% 100% 0
mgt-components.src.components.mgt-organization 47% 100% 0
mgt-components.src.components.mgt-person 78% 64% 0
mgt-components.src.components.mgt-person-card 56% 54% 0
mgt-components.src.components.mgt-picker 83% 100% 0
mgt-components.src.components.mgt-profile 40% 100% 0
mgt-components.src.components.mgt-tasks-base 87% 100% 0
mgt-components.src.components.mgt-theme-toggle 100% 100% 0
mgt-components.src.components.mgt-todo 79% 100% 0
mgt-components.src.components.sub-components.mgt-flyout 72% 53% 0
mgt-components.src.components.sub-components.mgt-spinner 100% 100% 0
mgt-components.src.graph 39% 89% 0
mgt-components.src.styles 92% 80% 0
mgt-components.src.utils 78% 12% 0
mgt-element.dist.es6.components.src.components 72% 73% 0
mgt-element.dist.es6.mock.src.mock 91% 73% 0
mgt-element.dist.es6.providers.src.providers 85% 76% 0
mgt-element.dist.es6.src 91% 80% 0
mgt-element.dist.es6.utils.src.utils 66% 69% 0
mgt-element.src 93% 40% 0
mgt-element.src.components 78% 100% 0
mgt-element.src.mock 81% 56% 0
mgt-element.src.providers 83% 91% 0
mgt-element.src.utils 71% 90% 0
Summary 65% (14014 / 21535) 64% (405 / 636) 0

Copy link

📖 The updated storybook is available here

@gavinbarron
Copy link
Member Author

@musale can you please take a look at this PR.

Copy link
Contributor

@musale musale left a comment

Choose a reason for hiding this comment

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

Looks good to me. There are nice refactors with this introduction that reduce code well.

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

Successfully merging this pull request may close these issues.

[BUG] Components are scheduling unnecessary updates and re-renders.
3 participants