Add new loading mode: local #6127

isaacroldan · 2025-07-17T15:15:45Z

WHY are these changes introduced?

To improve the handling of unknown extension types in local development mode, allowing the app to load successfully even when unknown extension types are present.

WHAT is this pull request doing?

Adds a new local mode to the AppLoaderMode type, which ignores errors for unknown extension types
Updates the localAppContext function to use the new local mode instead of strict

How to test your changes?

For know extensions only accessible when authenticated:

Create a local app with an admin_link extensions
Both app build and app deploy should work. build will ignore the admin_link extension

For completely unknown extensions:

Create a local app with an admin_link extension.
Change the type of the extension to "invalid_type"
app build still works because it ignore unknown extensions, app deploy should crash.

Measuring impact

How do we know this change was effective? Please choose one:

n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

Checklist

I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've considered possible documentation changes

isaacroldan · 2025-07-17T15:16:06Z

Add new loading mode: local #6127 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

github-actions · 2025-07-17T15:38:34Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	78.07% (+0.12% 🔼)	13002/16655
🟡	Branches	72.29% (+0.21% 🔼)	6339/8769
🟡	Functions	78.17% (+0.08% 🔼)	3370/4311
🟡	Lines	78.47% (+0.13% 🔼)	12311/15688

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / graphql-errors.ts	0%	0%	0%	0%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / app-context.ts	96.88%	88% (-1.29% 🔻)	100%	96.77%
🔴	`...` / base-command.ts	36.36% (-13.64% 🔻)	0%	100%	36.36% (-13.64% 🔻)

Test suite run success

3028 tests passing in 1311 suites.

Report generated by 🧪jest coverage report action from 5349676

MitchDickinson · 2025-07-17T16:56:03Z

packages/app/src/cli/models/app/loader.ts

@@ -466,6 +472,8 @@ class AppLoader<TConfig extends AppConfiguration, TModuleSpec extends ExtensionS

    if (specification) {
      usedKnownSpecification = true


It's not your change, but usedKnownSpecification seems pointless in this method since if there is no specification we always return early from the method, so there's no need to be checking usedKnownSpecification further down.

Actually, that's still useful when not using the local mode. It makes sense for the report mode.

MitchDickinson · 2025-07-17T17:12:35Z

packages/app/src/cli/models/app/loader.ts

@@ -466,6 +472,8 @@ class AppLoader<TConfig extends AppConfiguration, TModuleSpec extends ExtensionS

    if (specification) {
      usedKnownSpecification = true
+    } else if (this.mode === 'local') {
+      return undefined


Do we have a good sense of how the contents of the loaded app (the extensions list) are used throughout the CLI? If we knew this was only used by build it would be safer, but this loader could be used anywhere - how can we guarantee that all usages (current and future) will be OK with skipping an unknown extension?

That is tricky, for instance the only command that uses the report loader mode is the app info command, because we want to surface errors but still load the app.
But we can't prevent other commands from using that loading mode (unless we add a special eslint rule I guess). That's why we called it unsafeReportMode.

In this case, the local mode will only be used when using the localAppContext, which should only be used by unauthenticated commands (app build and app function).

MitchDickinson · 2025-07-17T17:25:01Z

packages/app/src/cli/services/app-context.ts

@@ -162,6 +161,6 @@ export async function localAppContext({
    directory,
    userProvidedConfigName,
    specifications,
-    mode: unsafeReportMode ? 'report' : 'strict',
+    mode: 'local',


I think since we dont' know who is using localAppContext, it's dangerous to assume we always want local here. I think we should pass an argument into localAppContext (similar to how it was before) so that the command can determine whether it's safe to skip unknown extensions when loading the local app. Since there might be cases where a default value of local is wrong and then bad things happen

Umm, my assumption was that if this is a local app, the expectation is that only local specifications can be used, and we should never crash in that case.

Since there might be cases where a default value of local is wrong and then bad things happen

I can't imagine a case where you would want to load the app with the local specs, but crash if there is an unknown extension 🤔. Right now the default mode is strict and that's what's causing the issues that made us make this change

The way I see it:
localAppContext -> local mode
linkedAppContext -> strict or report modes

isaacroldan marked this pull request as ready for review July 17, 2025 15:16

isaacroldan requested a review from a team as a code owner July 17, 2025 15:16

This comment has been minimized.

Sign in to view

isaacroldan force-pushed the 07-17-add_new_loading_mode_local branch 2 times, most recently from c3318c7 to 29f77cf Compare July 17, 2025 15:23

isaacroldan requested a review from a team as a code owner July 17, 2025 15:23

Add new loading mode: local

5349676

isaacroldan force-pushed the 07-17-add_new_loading_mode_local branch from 29f77cf to 5349676 Compare July 17, 2025 15:32

MitchDickinson reviewed Jul 17, 2025

View reviewed changes

gonzaloriestra approved these changes Jul 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add new loading mode: local #6127

Add new loading mode: local #6127

isaacroldan commented Jul 17, 2025 •

edited

Loading

Uh oh!

isaacroldan commented Jul 17, 2025

Uh oh!

This comment has been minimized.

github-actions bot commented Jul 17, 2025 •

edited

Loading

Uh oh!

MitchDickinson Jul 17, 2025

Uh oh!

isaacroldan Jul 18, 2025 •

edited

Loading

Uh oh!

MitchDickinson Jul 17, 2025

Uh oh!

isaacroldan Jul 18, 2025

Uh oh!

MitchDickinson Jul 17, 2025

Uh oh!

isaacroldan Jul 18, 2025

Uh oh!

Uh oh!

		@@ -466,6 +472,8 @@ class AppLoader<TConfig extends AppConfiguration, TModuleSpec extends ExtensionS

		if (specification) {
		usedKnownSpecification = true

Add new loading mode: local #6127

Are you sure you want to change the base?

Add new loading mode: local #6127

Conversation

isaacroldan commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Measuring impact

Checklist

Uh oh!

isaacroldan commented Jul 17, 2025

Uh oh!

This comment has been minimized.

github-actions bot commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report

Test suite run success

Uh oh!

MitchDickinson Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

isaacroldan Jul 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MitchDickinson Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

isaacroldan Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

MitchDickinson Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

isaacroldan Jul 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

isaacroldan commented Jul 17, 2025 •

edited

Loading

github-actions bot commented Jul 17, 2025 •

edited

Loading

isaacroldan Jul 18, 2025 •

edited

Loading