-
-
Notifications
You must be signed in to change notification settings - Fork 226
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(enhanced): Recursive search for versions of shared dependencies #3078
base: main
Are you sure you want to change the base?
Changes from all commits
99843fc
e6f1836
ea53f40
8f4c7de
9ca1e7c
4db7c35
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@module-federation/enhanced': minor | ||
--- | ||
|
||
Added recursively search for shared dependency versions |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -235,19 +235,29 @@ class ConsumeSharedPlugin { | |||
compilation.inputFileSystem, | ||||
context, | ||||
['package.json'], | ||||
(err, result) => { | ||||
(err, result, checkedDescriptionFilePaths) => { | ||||
if (err) { | ||||
requiredVersionWarning( | ||||
`Unable to read description file: ${err}`, | ||||
); | ||||
return resolve(undefined); | ||||
} | ||||
//@ts-ignore | ||||
const { data, path: descriptionPath } = result; | ||||
const { data } = result || {}; | ||||
if (!data) { | ||||
requiredVersionWarning( | ||||
`Unable to find description file in ${context}.`, | ||||
); | ||||
if (checkedDescriptionFilePaths) { | ||||
requiredVersionWarning( | ||||
[ | ||||
`Unable to find required version for "${packageName}" in description file/s`, | ||||
checkedDescriptionFilePaths.join('\n'), | ||||
'It need to be in dependencies, devDependencies or peerDependencies.', | ||||
].join('\n'), | ||||
); | ||||
} else { | ||||
requiredVersionWarning( | ||||
`Unable to find description file in ${context}.`, | ||||
); | ||||
} | ||||
|
||||
return resolve(undefined); | ||||
} | ||||
//@ts-ignore | ||||
|
@@ -259,15 +269,17 @@ class ConsumeSharedPlugin { | |||
data, | ||||
packageName, | ||||
); | ||||
if (typeof requiredVersion !== 'string') { | ||||
requiredVersionWarning( | ||||
`Unable to find required version for "${packageName}" in description file (${descriptionPath}). It need to be in dependencies, devDependencies or peerDependencies.`, | ||||
); | ||||
return resolve(undefined); | ||||
} | ||||
// @ts-ignore webpack internal semver has some issue, use runtime semver , related issue: https://github.com/webpack/webpack/issues/17756 | ||||
resolve(requiredVersion); | ||||
}, | ||||
({ data }) => { | ||||
const maybeRequiredVersion = | ||||
getRequiredVersionFromDescriptionFile(data, packageName); | ||||
return ( | ||||
data['name'] === packageName || | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if no version warning should still get logged There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same warning will be registered There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but i see this messge removed
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah, I thought about the self-referencing warning that doesn't get registered.
Maybe accumulate warnings for all description file names ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All good. I'll pull locally and touch it up a little then merge |
||||
typeof maybeRequiredVersion === 'string' | ||||
); | ||||
}, | ||||
); | ||||
}), | ||||
]).then(([importResolved, requiredVersion]) => { | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
it('should provide own dependency', async () => { | ||
expect(await import('lib')).toEqual( | ||
expect.objectContaining({ | ||
default: '[email protected] with [email protected]', | ||
}), | ||
); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"type": "commonjs" | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
"dependencies": { | ||
"lib": "^1.0.0" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
const { SharePlugin } = require('../../../../dist/src'); | ||
|
||
/** @type {import("../../../../").Configuration} */ | ||
module.exports = { | ||
context: `${__dirname}/cjs`, | ||
plugins: [ | ||
new SharePlugin({ | ||
shared: { | ||
lib: {}, | ||
transitive_lib: {}, | ||
}, | ||
}), | ||
], | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
it('should provide library from own package.json', async () => { | ||
expect(await import('lib1')).toEqual( | ||
expect.objectContaining({ | ||
default: '[email protected]', | ||
}), | ||
); | ||
}); | ||
|
||
it('should provide library from parent package.json', async () => { | ||
expect(await import('lib2')).toEqual( | ||
expect.objectContaining({ | ||
default: '[email protected]', | ||
}), | ||
); | ||
}); |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
"dependencies": { | ||
"lib2": "^2.0.0" | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
{ | ||
"dependencies": { | ||
"lib1": "^1.0.0", | ||
"lib2": "^1.0.0" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
const { SharePlugin } = require('../../../../dist/src'); | ||
|
||
/** @type {import("../../../../").Configuration} */ | ||
module.exports = { | ||
context: `${__dirname}/app1`, | ||
plugins: [ | ||
new SharePlugin({ | ||
shared: { | ||
lib1: {}, | ||
lib2: { | ||
singleton: true, | ||
}, | ||
}, | ||
}), | ||
], | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recursive lookup will stop under the following condition:
core/packages/enhanced/src/lib/sharing/utils.ts
Line 355 in ea53f40
and this warning will be registered
core/packages/enhanced/src/lib/sharing/ConsumeSharedPlugin.ts
Line 248 in ea53f40
but we can actually reach the root of fs. Maybe we should stop searching at some upwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.jsonThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. Okay well I think we can run with this. I'll review and merge. Then will port to rust