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

fix(externals): mismatched externals caused one-app to fail to start #90

Merged
merged 14 commits into from
Dec 14, 2021

Conversation

Matthew-Mallimo
Copy link
Member

A child module that requires an external that is NOT provided by its root module will cause one-app to fail to start up, or fail polling if already running.

Description

This fix works by catching an error that is thrown by onModuleLoad, a function passed to holocron from the one-app server. Typically this error would bubble up to one-app, where the module map polling would fail.

By catching the error, we are able to ignore the "bad" module by preventing it from being updated in the in-memory module map.

This means that if an existing module is updated to require an external that is NOT provided by the parent, we will use its last stable version until a working version of that module is deployed.

This does not affect the module map polling. It will continue to climb as normal up to the ONE_MAP_POLLING_MAX value.

Motivation and Context

This can prevent developers from starting the one-app server locally if a faulty module is deployed to the environment their one-app-runner config points to. It also prevents other modules from being updated in the module map until the faulty module is corrected.

How Has This Been Tested?

Pending unit tests

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (adding or updating documentation)
  • Dependency update

Checklist:

  • My change requires a change to the documentation and I have updated the documentation accordingly.
  • My changes are in sync with the code style of this project.
  • There aren't any other open Pull Requests for the same issue/update.
  • These changes should be applied to a maintenance branch.
  • This change requires cross browser checks.
  • This change impacts caching for client browsers.
  • This change adds additional environment variable requirements for Holocron users.
  • I have added the Apache 2.0 license header to any new files created.

What is the Impact to Developers Using Holocron?

Copy link
Member

@10xLaCroixDrinker 10xLaCroixDrinker left a comment

Choose a reason for hiding this comment

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

What are the implications of this in production? How will engineers know that a bad module was deployed? What happens on startup if the bad module is the root module?

@Matthew-Mallimo
Copy link
Member Author

Matthew-Mallimo commented Oct 12, 2021

What are the implications of this in production? How will engineers know that a bad module was deployed? What happens on startup if the bad module is the root module?

Since this just logs the errors, they would have to check the server logs to see if anything is wrong. The major sign that something is wrong is that their deployed version wont be reflected when they hit the page the module is on.

I haven't tested what would happen if the root module fails on server start, will report back on this.

Copy link
Contributor

@JAdshead JAdshead left a comment

Choose a reason for hiding this comment

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

unsure if we should reset the polling interval if a module fails to load.

Co-authored-by: Jonny Adshead <JAdshead@users.noreply.github.com>
@Matthew-Mallimo
Copy link
Member Author

unsure if we should reset the polling interval if a module fails to load.

If we do, the polling will always be 1 second ( or whatever the smallest polling interval is ) because it will repeatedly fail.

@JAdshead
Copy link
Contributor

unsure if we should reset the polling interval if a module fails to load.

If we do, the polling will always be 1 second ( or whatever the smallest polling interval is ) because it will repeatedly fail.

agreed, but if its in a broken state is waiting 5min to long

@github-actions github-actions bot added the test label Nov 22, 2021
@Matthew-Mallimo Matthew-Mallimo requested a review from a team November 22, 2021 14:43
@10xLaCroixDrinker 10xLaCroixDrinker marked this pull request as ready for review November 22, 2021 17:14
@10xLaCroixDrinker 10xLaCroixDrinker requested a review from a team as a code owner November 22, 2021 17:14
return addHigherOrderComponent(loadedModule);
} catch (e) {
// eslint-disable-next-line no-console
console.error(e);
Copy link
Member

Choose a reason for hiding this comment

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

Add some details/context to this error for the users

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to bloat this PR but we should consider adding URLs to our error messages

Copy link
Contributor

@infoxicator infoxicator left a comment

Choose a reason for hiding this comment

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

LGTM, we still need to add integration tests for the new scenarios once this PR gets merged and we update holocron in One App

Co-authored-by: Jonny Adshead <JAdshead@users.noreply.github.com>
@10xLaCroixDrinker 10xLaCroixDrinker requested a review from a team December 6, 2021 18:27
@10xLaCroixDrinker 10xLaCroixDrinker merged commit fe49f8c into main Dec 14, 2021
@10xLaCroixDrinker 10xLaCroixDrinker deleted the fix/externalsCrash branch December 14, 2021 19:18
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants