MemberJunction / MemberJunction/MJ

GetGlobalObjectStore throws and catches a ReferenceError on every call in Node (~1.4µs), taxing every BaseSingleton .Instance access

Open
#3,555 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
TSQL
Stars
29
Forks
6
Avg merge
2d 1h
Merged PRs (30d)
323

Description

## Summary

`GetGlobalObjectStore()` ([`packages/MJGlobal/src/util.ts:18`](https://github.com/MemberJunction/MJ/blob/next/packages/MJGlobal/src/util.ts#L18)) probes a **bare `window` identifier** inside a `try`. In Node that is not a falsy read — it throws a `ReferenceError`, which the `catch` then swallows before falling back to `global`.

```ts
export function GetGlobalObjectStore(): GlobalObjectStore | null {
try {
if (window) // ← ReferenceError in Node, on EVERY call
return window as unknown as GlobalObjectStore;
else {
if (global) return global as unknown as GlobalObjectStore;
else return null;
}
}
catch (e) {
try {
if (global) return global as unknown as GlobalObjectStore;
...
```

Throwing and catching an exception costs roughly **45x** the property reads it is guarding, and this function sits underneath `BaseSingleton.getInstance()` — so **every `.Instance` access anywhere in MJ pays it, on every request**, server-side.

## Measurements

Node v24.13.0, darwin-arm64, microbenchmarks against the built `dist`:

| Call | ns/op |
|---|---|
| `GetGlobalObjectStore()` — current implementation | **1,434** |
| Same semantics with a `typeof` guard (no throw) | **32** |
| → the exception's share | **97.8%** |
| `LocalCacheManager.Instance` (a single `BaseSingleton.getInstance`) | **1,983** |
| `GetDataHooks('PostRunView')` with 0 hooks registered | **1,645** |

For scale: a 383-row `LocalCacheManager.GetRunViewResult()` cache hit measures 2,534 ns, so a *single* `.Instance` access costs about 78% of the cache lookup it is usually helping perform.

## Scope

- `BaseSingleton.getInstance()` and `BaseSingleton.GetGlobalObjectStore()` ([`packages/MJGlobal/src/BaseSingleton.ts:16,39,51`](https://github.com/MemberJunction/MJ/blob/next/packages/MJGlobal/src/BaseSingleton.ts)) — so every MJ singleton access
- 83 files reference `GetGlobalObjectStore` directly

**Browsers are unaffected** — `window` exists there, so the throw path is Node-only. This is purely a server-side cost.

## Proposed fix

Replace the exception-driven probe with an explicit existence check. `globalThis` is available in every environment MJ targets (Node 12+, all modern browsers) and removes the branching entirely:

```ts
export function GetGlobalObjectStore(): GlobalObjectStore | null {
if (typeof globalThis !== 'undefined' && globalThis) {
return globalThis as unknown as GlobalObjectStore;
}
return null;
}
```

If preserving the exact `window`-before-`global` preference matters (it should not — in a browser `globalThis === window`, and in Node `globalThis === global`), the conservative variant is:

```ts
if (typeof window !== 'undefined' && window) return window as unknown as GlobalObjectStore;
if (typeof global !== 'undefined' && global) return global as unknown as GlobalObjectStore;
return null;
```

Both are semantically identical to today's behavior in every environment; only the cost differs.

## Risk

Low but broad. The function backs the Global Object Store that guarantees singleton identity across duplicated module copies, so the change should be verified to return the *same object identity* as before in Node, browser, and Vitest environments — a returned-identity test plus the existing `BaseSingleton` suites should cover it.

## Context

Found while measuring the cost of running `PostRunView` data hooks on cache-hit paths in #3425. That PR works around the cost locally by memoizing the resolved store inside `dataHooks.ts`; this issue is the source fix, deliberately kept out of that PR because the blast radius is repo-wide.

Contributor guide

Open the contributing guide

Research direction

Start with packages/MJGlobal/src/util.ts and packages/MJGlobal/src/BaseSingleton.ts, focusing on GetGlobalObjectStore and BaseSingleton.getInstance. Verify the returned global-store identity in Node, browser, and Vitest environments, then run the existing BaseSingleton suites. Done means the lookup preserves singleton identity without the Node exception path and the suites remain passing.

Written by the indexing model from the issue text.

Assessment

Tech stack
node.js, typescript
Domain
backend, performance
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.