feat(enhanced): Recursive search for versions of shared dependencies by barabaiiika · Pull Request #3078 · module-federation/core

barabaiiika · 2024-10-14T12:23:29Z

Description

Improved recursive version search of shared dependencies for cases where there is a parent package.json containing the required version. This is specific to monorepos and dual packages(esm/cjs), where the closest package.json is only used to specify the package type.

There is already an open pull request that solves this #2681, but it has a recursive call to getDescriptionFile, which already calls itself recursively. I just added an extra check that the package.json found contains a version description.

Related Issue

#2680
webpack/webpack#13457
This is also mentioned in the discussion #2917

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.

changeset-bot · 2024-10-14T12:23:34Z

🦋 Changeset detected

Latest commit: 32478b9

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

This PR includes changesets to release 27 packages

Name	Type
@module-federation/enhanced	Patch
@module-federation/modern-js	Patch
@module-federation/nextjs-mf	Patch
@module-federation/node	Patch
@module-federation/rsbuild-plugin	Patch
@module-federation/storybook-addon	Patch
@module-federation/modernjsapp	Patch
@module-federation/runtime	Patch
@module-federation/rspack	Patch
@module-federation/webpack-bundler-runtime	Patch
@module-federation/sdk	Patch
@module-federation/runtime-tools	Patch
@module-federation/managers	Patch
@module-federation/manifest	Patch
@module-federation/dts-plugin	Patch
@module-federation/third-party-dts-extractor	Patch
@module-federation/devtools	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/retry-plugin	Patch
@module-federation/data-prefetch	Patch
@module-federation/error-codes	Patch
@module-federation/esbuild	Patch
@module-federation/utilities	Patch
website-new	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

netlify · 2024-10-14T12:23:46Z

✅ Deploy Preview for module-federation-docs ready!

Name	Link
🔨 Latest commit	`32478b9`
🔍 Latest deploy log	https://app.netlify.com/sites/module-federation-docs/deploys/672c008560a3dd00087336c3
😎 Deploy Preview	https://deploy-preview-3078--module-federation-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

ScriptedAlchemy · 2024-10-14T23:01:28Z

packages/enhanced/src/lib/sharing/ConsumeSharedPlugin.ts

                    data,
                    packageName,
                  );
-                  if (typeof requiredVersion !== 'string') {


we should still keep the required version warning if the recursive lookup doesnt return the package for whatever reason.

What happens if someone has a package.json outside their app project, like in the root of their FS? This will crawl upwards across the whole fs?

recursive lookup will stop under the following condition:

core/packages/enhanced/src/lib/sharing/utils.ts

Line 355 in ea53f40

if (!parentDirectory || parentDirectory === directory) {

and this warning will be registered

core/packages/enhanced/src/lib/sharing/ConsumeSharedPlugin.ts

Line 248 in ea53f40

`Unable to find description file in ${context}.`,

but we can actually reach the root of fs. Maybe we should stop searching at some upwards?

yeah, maybe try with compiler.context as the edge boundary? Im not sure if that will work in your use case, but it probbably will be a good idea to somehow prevent it from searching the entire fs all the way up to root if it doesnt find something. Like keep it from leaving the current repo, usually i use compiler.context to find the boundary of the compiler.

I don't have any ideas yet on how to limit the search tree upwards😔 compiler.context will help with dual-packages, but for monorepos search should go up to the monorepo package.json

Yeah. Okay well I think we can run with this. I'll review and merge. Then will port to rust

ScriptedAlchemy · 2024-10-14T23:01:47Z

packages/enhanced/src/lib/sharing/ConsumeSharedPlugin.ts

+                  const maybeRequiredVersion =
+                    getRequiredVersionFromDescriptionFile(data, packageName);
+                  return (
+                    data['name'] === packageName ||


if no version warning should still get logged

same warning will be registered

but i see this messge removed

Unable to find required version for "${packageName}" in description file (${descriptionPath}). It need to be in dependencies, devDependencies or peerDependencies.,
);

will same message report or does it fall back to unable to find description file only?

Oh yeah, I thought about the self-referencing warning that doesn't get registered.

core/packages/enhanced/src/lib/sharing/ConsumeSharedPlugin.ts

Line 256 in 5ac093d

return resolve(undefined);

Maybe accumulate warnings for all description file names (descriptionPath ) visited during recursive lookup to create a summary warning?

Good idea, if you want to update the PR to do so, feel free. Otherwise just make sure that it reports back that warning if it comes up dry, rather than removing it completely. Its useful to tell dev they should list the package in their deps etc if it cant find one, especially now that it can recurse upwards

I added accumulation of checked files to the warning if the version is not found in them. I think the output looks clearer now. But the implementation looks a bit ugly 😅

All good. I'll pull locally and touch it up a little then merge

…dependencies

…l_mode' into feat/search_package_json_for_dual_mode

ScriptedAlchemy · 2024-10-31T18:23:07Z

@barabaiiika i have updated this PR, please double check it on your end

barabaiiika · 2024-11-01T08:51:37Z

packages/enhanced/src/lib/sharing/utils.ts

+  // Create an object to hold the function and the checkedFilePaths
+  const satisfiesDescriptionFileDataInternal = {
+    check: satisfiesDescriptionFileData,
+    checkedFilePaths: new Set<string>(),


@ScriptedAlchemy but this will create a new Set instance for each getDescriptionFile call and only the filepaths from the last instance will be passed to the callback?

Okay i added it as a default to the function, then pass the same set recursively down.

barabaiiika · 2024-11-01T09:00:31Z

packages/enhanced/src/lib/sharing/utils.ts

        return callback(
          null,
          undefined,
-          satisfiesDescriptionFileDataInternal?.checkedFilePaths &&


calling code relies on the fact that checkedDescriptionFilePaths may be undefined:

core/packages/enhanced/src/lib/sharing/ConsumeSharedPlugin.ts

Line 244 in c0ef1cd

if (checkedDescriptionFilePaths) {

maybe it's worth adding a length check to the checkedDescriptionFilePaths array now?

@barabaiiika could you help me update this part, dont fully understand.

I added a check on the length of the array of checked file paths to select the appropriate warning.

ScriptedAlchemy · 2024-11-05T06:44:47Z

@barabaiiika kind bump ✨️

barabaiiika · 2024-11-05T07:29:23Z

@barabaiiika kind bump ✨️

Sorry, I've been busy the last few days. I'll start today👌

ScriptedAlchemy · 2024-11-05T07:50:10Z

Thank you!

ScriptedAlchemy · 2024-11-05T22:39:40Z

packages/enhanced/src/lib/sharing/ConsumeSharedPlugin.ts

                  const { data } = /** @type {DescriptionFile} */ result || {};
                  if (!data) {
-                    if (checkedDescriptionFilePaths) {
+                    if (checkedDescriptionFilePaths?.length) {


well, i feel stupid

It's okay, it's my mistake - I didn't explain it well☺️

I'll merge this tomorrow. Then get it synced with rspack and I've already opened the pull request to webpack core as well. Appreciate the contribution. This has definitely been long overdue

feat(enhanced): recursive search for versions of shared dependencies

99843fc

github-actions bot added the change: feat label Oct 14, 2024

Merge branch 'main' into feat/search_package_json_for_dual_mode

e6f1836

ScriptedAlchemy reviewed Oct 14, 2024

View reviewed changes

ScriptedAlchemy and others added 4 commits October 14, 2024 16:46

Merge branch 'main' into feat/search_package_json_for_dual_mode

ea53f40

feat(enhanced): cumulative warning about missing package version in *…

8f4c7de

…dependencies

Merge remote-tracking branch 'origin/feat/search_package_json_for_dua…

9ca1e7c

…l_mode' into feat/search_package_json_for_dual_mode

Merge branch 'main' into feat/search_package_json_for_dual_mode

4db7c35

barabaiiika requested a review from ScriptedAlchemy October 21, 2024 18:37

chore: remove old tsignores

e57ba5f

refactor: pass stateful refs

c0ef1cd

ScriptedAlchemy force-pushed the feat/search_package_json_for_dual_mode branch from e46e637 to c0ef1cd Compare October 31, 2024 18:26

barabaiiika commented Nov 1, 2024

View reviewed changes

fix: pass same set down

0b2a172

barabaiiika and others added 2 commits November 5, 2024 21:44

Merge branch 'main' into feat/search_package_json_for_dual_mode

2b9577b

fix(enhanced): added check for existence of visited files

b07090f

ScriptedAlchemy reviewed Nov 5, 2024

View reviewed changes

ScriptedAlchemy added 2 commits November 5, 2024 14:42

Update smooth-goats-knock.md

ec4e2ce

Merge branch 'main' into feat/search_package_json_for_dual_mode

32478b9

ScriptedAlchemy approved these changes Nov 7, 2024

View reviewed changes

ScriptedAlchemy merged commit 47fdbc2 into module-federation:main Nov 7, 2024

barabaiiika deleted the feat/search_package_json_for_dual_mode branch November 7, 2024 04:53

2heal1 mentioned this pull request Nov 7, 2024

Release v0.7.1 #3192

Merged

ScriptedAlchemy mentioned this pull request Nov 7, 2024

[Feature]: Module Federation: Recursive share version search/resolve web-infra-dev/rspack#8375

Closed

Uh oh!

Conversation

barabaiiika commented Oct 14, 2024

Description

Related Issue

Types of changes

Checklist

Uh oh!

changeset-bot bot commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

netlify bot commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for module-federation-docs ready!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScriptedAlchemy commented Oct 31, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScriptedAlchemy commented Nov 5, 2024

Uh oh!

barabaiiika commented Nov 5, 2024

Uh oh!

ScriptedAlchemy commented Nov 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot bot commented Oct 14, 2024 •

edited

Loading

netlify bot commented Oct 14, 2024 •

edited

Loading