Skip to content

ACT-2120: Prevent double initialization of mvc controllers #19

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+21 −5
Open

ACT-2120: Prevent double initialization of mvc controllers #19

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12d0a5d
Prevent double initialization by adding an initializing state and run…
elbunuelo Aug 20, 2025
d3f03c2
Update src/controller/ApplicationController.tsx
elbunuelo Aug 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 21 additions & 5 deletions src/controller/ApplicationController.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,13 @@ type GetControllerConstructor<T> = { new (): T };
type GetControllerProps<T extends ApplicationControllerConstructor<any>> =
T extends ApplicationControllerConstructor<infer P> ? P : never;

type InitializationState = 'uninitialized' | 'initializing' | 'initialized';
const InitializationStates: { [k: string]: InitializationStates } = {
INITIALIZING: 'initializing',
UNINITIALIZED: 'uninitialized',
INITIALIZED: 'initialized',
} as const;

/**
* General rules to follow for using controllers:
*
Expand All @@ -51,7 +58,7 @@ class ApplicationController<
Parent extends ApplicationController<any, any, any> = any,
> {
id: string;
initialized: boolean;
initializationState: InitializationState;
parent: Parent;
state: State;
proxiedThis: any;
Expand All @@ -62,7 +69,7 @@ class ApplicationController<

constructor() {
this.id = randomId();
this.initialized = false;
this.initializationState = InitializationStates.UNINITIALIZED;
this.parent = null;
this.state = undefined;
this.runOnDestroy = [];
Expand Down Expand Up @@ -124,7 +131,8 @@ class ApplicationController<
* @hidden
*/
internalInitialize(parentController: Parent, initialArgs: Props) {
if (!this.initialized) {
if (this.initializationState === InitializationStates.UNINITIALIZED) {
this.initializationState = InitializationStates.INITIALIZING;
this.parent = parentController;

debug(
Expand All @@ -138,8 +146,8 @@ class ApplicationController<

this.state = store(cloneDeep(this.initialState));
if (this.initialize) this.initialize(initialArgs);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line is at least partially related to the problem. It's possible for the initialize method to throw an error, and I think in that case, we might get into a bad state.

But also, I think the problem is if initialize makes changes that cause a re-render during the initial render? Can/should we run the internalInitialize in a different way perhaps?

this.initialized = true;
} else {
this.initializationState = InitializationStates.INITIALIZED;
} else if (this.initializationState === InitializationStates.INITIALIZED) {
const oldProps = { ...this.props };
Object.keys(initialArgs).forEach(key => {
if (this.props[key] !== initialArgs[key]) {
Expand All @@ -148,9 +156,17 @@ class ApplicationController<
});

this.changeProps(initialArgs, oldProps);
} else {
throw new Error(
'Attempted to reinitialize controller before initialization finished.'
);
Comment on lines +160 to +162
Copy link

Choose a reason for hiding this comment

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

Maybe just logging a warning would be more appropriate? Depends how this was happening I suppose but I worry throwing an error will cause any later/random other code to not be executed?

}
}

get initialized() {
return this.initializationState === InitializationStates.INITIALIZED;
}

/**
* Controllers can override this method to cleanup when removed
*/
Expand Down