Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.2k
More intelligently guess the required version of shared modules#11359
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
In the package.json, devDependencies is arguably higher precedence since it records what actually gets installed.
…module.Sometimes a module is installed with a local path instead of a version. If we do not recognize a version number, this checks to see if it is a path on disk. If it is, we get the required version from the source on disk.Perhaps a better way to handle this situation is getting the actual installed version of the module. That would handle more exotic ways of installing, including from git, etc.
webpack-bot commentedAug 20, 2020
For maintainers only:
|
jasongrout commentedAug 20, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Should we also check here:
if the configured requireVersion is valid and parse it, or just continue passing it through? |
jasongrout commentedAug 20, 2020
Ah, I see that it is parsed above, though I'm not sure if the config in the init is the same as the config in the apply. Should we check it above at
../mypackage, for example? |
webpack-bot commentedAug 20, 2020
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
jasongrout commentedAug 20, 2020
I added a validity check for strings ind620fce |
webpack-bot commentedAug 21, 2020
It looks like this Pull Request doesn't includeenough test cases (based on Code Coverage analysis of the PR diff). A PR need to be covered by tests if you add a new feature (we want to make sure that your feature is working) or if you fix a bug (we want to make sure that we don't run into a regression in future). @jasongrout Please check if this is appliable to your PR and if you can add more test cases. Read thetest readme for details how to write test cases. |
jasongrout commentedAug 21, 2020
I looked, but I'm still not sure where similar test cases are, or where I should add a test case. Are there test cases for similar scenarios somewhere in the code? |
In current module federation, the required version of a shared module defaults to the version requirement in package.json. However, sometimes a module is installed from the local filesystem (for example, when testing a local copy).
Currently, if the dependency in package.json looks like
"mypackage": "../mypackage", then the string is parsed as a version number leading to a nonsensical required version. This is a bug in master.This change first checks to see if the required version is a valid range, and if not, tries to resolve the path on disk and get the version number from the path on disk.
This does not handle more exotic ways of installing packages, such as with github urls, etc. Perhaps an even better approach is to try to resolve the package name to find the version installed, and use that version number.
In order to resolve paths on the filesystem, I also added a simplified optional
resolvemethod to the filesystem interface. Join does not work since paths can be relative or absolute.What kind of change does this PR introduce?
bugfix and feature enhancement
Did you add tests for your changes?
I don't know where I should add a test. Where should one go?
Does this PR introduce a breaking change?
What needs to be documented once your changes are merged?