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

fix(runtime): set loading if registeredShared not set #2943

Merged
merged 3 commits into from
Sep 11, 2024

Conversation

2heal1
Copy link
Member

@2heal1 2heal1 commented Sep 11, 2024

Description

set loading if registeredShared not set

Related Issue

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I have updated the documentation.

Copy link

changeset-bot bot commented Sep 11, 2024

🦋 Changeset detected

Latest commit: 2fbd6cb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 37 packages
Name Type
@module-federation/runtime Patch
@module-federation/devtools Patch
@module-federation/dts-plugin Patch
@module-federation/nextjs-mf Patch
@module-federation/node Patch
@module-federation/retry-plugin Patch
@module-federation/runtime-tools Patch
@module-federation/webpack-bundler-runtime Patch
@module-federation/modern-js Patch
@module-federation/enhanced Patch
@module-federation/rspack Patch
modernjs-ssr-dynamic-nested-remote Patch
modernjs-ssr-dynamic-remote-new-version Patch
modernjs-ssr-dynamic-remote Patch
modernjs-ssr-host Patch
modernjs-ssr-nested-remote Patch
modernjs-ssr-remote-new-version Patch
modernjs-ssr-remote Patch
3008-runtime-remote Patch
host Patch
host-v5 Patch
host-vue3 Patch
remote1 Patch
remote2 Patch
remote3 Patch
remote4 Patch
@module-federation/modernjs Patch
@module-federation/sdk Patch
@module-federation/managers Patch
@module-federation/manifest Patch
@module-federation/third-party-dts-extractor Patch
@module-federation/bridge-react Patch
@module-federation/bridge-vue3 Patch
@module-federation/bridge-shared Patch
@module-federation/bridge-react-webpack-plugin Patch
@module-federation/esbuild Patch
@module-federation/utilities Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@squadronai squadronai bot left a comment

Choose a reason for hiding this comment

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

Summary

This pull request introduces a fix to the runtime module's shared resource handling and a new type for the module federation plugin in the SDK. The key changes are:

  • In the SharedHandler class, the loading flag is now set if the registeredShared property is not set. This ensures that the loading state is properly tracked for shared resources, which is important for managing their lifecycle in the runtime.
  • A new 'module-import' type has been added to the ExternalsType union in the ModuleFederationPlugin of the SDK. This change likely provides more flexibility and control over how external modules are loaded and resolved at runtime in a module federation setup.

These changes aim to improve the overall stability and functionality of the runtime and module federation features in the codebase.

File Summaries
File Summary
packages/runtime/src/shared/index.ts The code changes introduce a fix to set the loading flag in the SharedHandler class if the registeredShared property is not set. This ensures that the loading state is properly tracked for shared resources, which is important for managing the lifecycle of these resources in the runtime.
packages/sdk/src/types/plugins/ModuleFederationPlugin.ts The code changes introduce a new 'module-import' type to the ExternalsType union, which is used to specify the type of external dependencies in a module federation setup. This change likely aims to provide more flexibility and control over how external modules are loaded and resolved at runtime.

Copy link

netlify bot commented Sep 11, 2024

Deploy Preview for module-federation-docs ready!

Name Link
🔨 Latest commit 2fbd6cb
🔍 Latest deploy log https://app.netlify.com/sites/module-federation-docs/deploys/66e13c0294049a0008f10e4e
😎 Deploy Preview https://deploy-preview-2943--module-federation-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Contributor

@squadronai squadronai bot left a comment

Choose a reason for hiding this comment

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

Incremental Review

Comments posted: 1

Configuration

Squadron Mode: essential

Commits Reviewed

2394e38a989077b87e42c48fbf4918dbb56b93fa...06a4f693f5e84fe62c24aeba43266e1c9adabd0a

Files Reviewed
  • packages/runtime/src/shared/index.ts
Files Ignored

These files were ignored due to the filter in the squadron.yaml file.

  • .changeset/rude-trees-ring.md

Comment on lines +502 to 504
if (loading && !registeredShared.loading) {
registeredShared.loading = loading;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider updating the condition to set loading even if registeredShared.loading is already set. This ensures that the most up-to-date loading state is always reflected. Here's a suggested change:

Suggested change
if (loading && !registeredShared.loading) {
registeredShared.loading = loading;
}
if (loading) {
registeredShared.loading = loading;
}

This modification allows the loading state to be updated even if it was previously set, which could be useful in scenarios where the loading state might change during the lifecycle of the shared module.

packages/runtime/src/shared/index.ts Dismissed Show dismissed Hide dismissed
Copy link
Contributor

@squadronai squadronai bot left a comment

Choose a reason for hiding this comment

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

Incremental Review

Comments posted: 0

Configuration

Squadron Mode: essential

Commits Reviewed

06a4f693f5e84fe62c24aeba43266e1c9adabd0a...def2081d2a835d89676c8902c89d428b392dde6d

Files Reviewed
  • packages/sdk/src/types/plugins/ModuleFederationPlugin.ts
Files Ignored

These files were ignored due to the filter in the squadron.yaml file.

  • .changeset/rude-trees-ring.md
  • packages/storybook-addon/src/lib/storybook-addon.ts

@zhoushaw zhoushaw merged commit 9f98292 into main Sep 11, 2024
17 checks passed
@zhoushaw zhoushaw deleted the fix/runtime-shared-status branch September 11, 2024 08:35
@2heal1 2heal1 mentioned this pull request Sep 11, 2024
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.

2 participants