Skip to content
This repository has been archived by the owner on May 3, 2024. It is now read-only.

fix(provided-externals): logs warning for child modules #32

Merged
merged 9 commits into from
Mar 26, 2020
Merged

fix(provided-externals): logs warning for child modules #32

merged 9 commits into from
Mar 26, 2020

Conversation

Francois-Esquire
Copy link
Contributor

Replaced error thrown onModuleLoad with warning log for child modules that contain the providedExternals in appConfig.

@Francois-Esquire Francois-Esquire requested a review from a team February 10, 2020 17:34
@oneamexbot
Copy link
Contributor

oneamexbot commented Feb 10, 2020 •
edited
Loading

📊 Bundle Size Report

file name size on disk gzip
app.js 112.6KB 31.4KB
runtime.js 15KB 5.3KB
vendors.js 128.1KB 37.9KB
app~vendors.js 403.5KB 105.2KB
legacy/app.js 119.4KB 33KB
legacy/runtime.js 15KB 5.3KB
legacy/vendors.js 163KB 44.8KB
legacy/app~vendors.js 409.8KB 106.9KB

Generated by 🚫 dangerJS against f0d86b5

@Francois-Esquire
Copy link
Contributor Author

☑️ Preflight Checklist:

What is the communication plan for this change?
None, providedExternals pending documentation

Does any documentation need to be updated or added to account for this change? If so was it done already?
No

What is the motivation for this change?
To prevent the app from crashing based on child module mis-configuration

Should these changes also be applied to a maintenance branch or any other long lived branch?
No

How was this change tested?
Unit and integration testing

Does this change require cross browser checks? Why or why not?
No, server side behavior

Does this change require a performance test prior to merging? Why or why not?
No, replaced throwing an error with console.warn

Could this be considered a breaking change? Why or why not?
No, API is unchanged

Does the change impact caching?
No

Does the change impact HTTP headers?
No

Does the change have any new infrastructure requirements?
No

Does the change affect other versions of the app?
No

Does the change require additional environment variables?
No

What is the impact to tenants?
None

What is the impact to individual users?
Warning on mis-configuration of appConfig

What is the change to the size of assets?
None

Should integration tests be added to protect against future regressions on this change?
Added

10xLaCroixDrinker
10xLaCroixDrinker suggested changes Feb 10, 2020
View reviewed changes
prod-sample/sample-modules/late-frank/0.0.1/src/index.js Outdated Show resolved Hide resolved
__tests__/integration/one-app.spec.js Outdated Show resolved Hide resolved
src/server/utils/onModuleLoad.js
throw new Error(`Module ${moduleName} attempted to provide externals, but it is not the root module.`);
console.warn(
`Module ${moduleName} attempted to provide externals. Only the root module can provide externals.`
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will need some changes around here: https://github.com/americanexpress/one-app-cli/blob/master/packages/one-app-bundler/webpack/loaders/provided-externals-loader.js#L34

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's best we move that helper added by the loader into One App -> getRootModule and remove it from being injected. Thoughts?

Copy link
Contributor

@mtomcal mtomcal Feb 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Francois-Esquire Francois-Esquire force-pushed the bugfix/provided-externals-child-error branch from 66521f5 to aa0b3a1 Compare February 24, 2020 21:23
mtomcal
mtomcal previously approved these changes Feb 25, 2020
View reviewed changes
JamesSingleton
JamesSingleton previously approved these changes Feb 28, 2020
View reviewed changes
10xLaCroixDrinker
10xLaCroixDrinker suggested changes Mar 3, 2020
View reviewed changes
Copy link
Member

@10xLaCroixDrinker 10xLaCroixDrinker left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There need to be tests that ensure the dep from the right module is used. Maybe this could be done by providing different externals.

nellyk
nellyk previously approved these changes Mar 13, 2020
View reviewed changes
@Francois-Esquire Francois-Esquire merged commit 24a9be4 into master Mar 26, 2020
@Francois-Esquire Francois-Esquire deleted the bugfix/provided-externals-child-error branch March 26, 2020 16:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mtomcal mtomcal mtomcal approved these changes

@10xLaCroixDrinker 10xLaCroixDrinker 10xLaCroixDrinker approved these changes

@JamesSingleton JamesSingleton JamesSingleton left review comments

@nellyk nellyk nellyk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Francois-Esquire @oneamexbot @10xLaCroixDrinker @mtomcal @nellyk @JamesSingleton