feat(enhanced): Recursive search for versions of shared dependencies #3078
Conversation
🦋 Changeset detectedLatest commit: 4db7c35 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 packages
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 |
✅ Deploy Preview for module-federation-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -259,15 +258,17 @@ class ConsumeSharedPlugin { | |||
| data, | |||
| packageName, | |||
| ); | |||
| if (typeof requiredVersion !== 'string') { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
recursive lookup will stop under the following condition:
and this warning will be registered
but we can actually reach the root of fs. Maybe we should stop searching at some upwards?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yeah. Okay well I think we can run with this. I'll review and merge. Then will port to rust
| const maybeRequiredVersion = | ||
| getRequiredVersionFromDescriptionFile(data, packageName); | ||
| return ( | ||
| data['name'] === packageName || |
There was a problem hiding this comment.
if no version warning should still get logged
There was a problem hiding this comment.
same warning will be registered
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Oh yeah, I thought about the self-referencing warning that doesn't get registered.
Maybe accumulate warnings for all description file names (descriptionPath ) visited during recursive lookup to create a summary warning?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
All good. I'll pull locally and touch it up a little then merge
…l_mode' into feat/search_package_json_for_dual_mode
Description
Improved recursive version search of shared dependencies for cases where there is a parent
package.jsoncontaining the required version. This is specific to monorepos and dual packages(esm/cjs), where the closestpackage.jsonis 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 thepackage.jsonfound contains a version description.Related Issue
#2680
webpack/webpack#13457
This is also mentioned in the discussion #2917
Types of changes
Checklist