Skip to content
This repository has been archived by the owner on Nov 20, 2023. It is now read-only.

fix(deps): Add webpack-sources dependency #397

Merged
merged 2 commits into from
Oct 6, 2022

Conversation

lobsterkatie
Copy link
Member

@lobsterkatie lobsterkatie commented Oct 6, 2022

In #307, a dependency on webpack-sources was added to the code, but not to package.json. From that PR description:

Adding webpack-sources as dependency breaks the tests (some of the tests use older version(s) of node than webpack-sources requires as minimum). On the other hand, if the user is using module federation, their webpack version is at least 5 and webpack-sources come with it. So, adding webpack-sources as a dependency is not a must. For the same reason, I did not put the require statement at the top of the file (it might be missing for users using an older version of webpack)

Though this logic is generally sound, it (specifically the "their webpack version is at least 5 and webpack-sources come with it" part) doesn't account for cases in which webpack is being consumed from a single bundle. In those cases, the code from webpack-sources exists in the user's dev environment, but the actual webpack-sources package doesn't. (This is true, for example, in nextjs, which pre-compiles many of its dependencies, including webpack, into minified bundles.) This can lead to errors when trying to use module federation alongside our nextjs SDK.

To fix this, webpack-sources has been added as a first-class runtime dependency. Because that guarantees that it will be present and require-able, its import has been moved out of a mid-file try-catch to instead be a legit import at the top of the file in which it's used.

@lobsterkatie lobsterkatie merged commit e04db0a into master Oct 6, 2022
@lobsterkatie lobsterkatie deleted the kmclb-add-webpack-sources-dep branch October 6, 2022 08:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants