-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add proper localStorage availability checks to support Node v25 in @typescript/vfs #3450
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.
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
base: v2
Are you sure you want to change the base?
Conversation
This could be fine but requires a changeset to make a release. Run |
I somewhat expect that we need to detect this another way, though. |
@jakebailey Sure! Let's analyze this. Consider the following console.log('localStorage type:', typeof localStorage);
console.log('localStorage value:', localStorage);
console.log('localStorage keys:', Object.keys(localStorage));
console.log('getItem type:', typeof localStorage.getItem);
console.log('instanceof Storage:', localStorage instanceof Storage);
console.log('prototype methods:', Object.getOwnPropertyNames(Object.getPrototypeOf(localStorage)));
try {
localStorage.getItem('test');
console.log('✅ Works');
} catch (e) {
console.log('❌ Error:', e.message);
} In Node 25, (1)
(2)
Then, in Node 24 via
This proves that Node 25 introduces a case where In this PR I proposed directly checking for the existence of the I can think of a few solutions.
interface LocalStorageLike {
getItem(key: string): string | null;
setItem(key: string, value: string): void;
removeItem(key: string): void;
}
declare var localStorage: LocalStorageLike | undefined;
declare var fetch: FetchLike | undefined;
declare var Storage: any;
let hasLocalStorage = false;
try {
hasLocalStorage =
typeof localStorage !== `undefined` &&
typeof Storage !== `undefined` &&
localStorage instanceof Storage;
} catch (error) {}
const hasProcess = typeof process !== `undefined`;
const shouldDebug =
(hasLocalStorage && localStorage!.getItem("DEBUG")) ||
(hasProcess && process.env.DEBUG);
const debugLog = shouldDebug
? console.log
: (_message?: any, ..._optionalParams: any[]) => ""; This works, but I'm not sure about BC (though I tested it in Node 24 and Node 25 with both flags, and it works in all cases). Also, my suggestion here is not the prettiest since it uses
declare var localStorage: LocalStorageLike | undefined | Record<string, never>; This is a more fair type declaration in Node 25 because it considers the case the object can be empty.:
Then, we can either (2a) directly check for the function(s) we need inside the let hasLocalStorage = false;
try {
hasLocalStorage =
typeof localStorage !== `undefined` &&
typeof localStorage.getItem === `function`; // Potentially check for `setItem` and `removeItem` too
} catch (error) {} I like this solution because it's as tight as we can get which makes it resilient. However, if you want to avoid direct let hasLocalStorage = false;
try {
hasLocalStorage =
typeof localStorage !== `undefined` && Object.getPrototypeOf(localStorage) !== Object.prototype;
} catch (error) {} I'm leaning towards (2a) for the reasons stated above, which one sounds better to you? |
Honestly, what you have in the PR already is probably fine. It just needs the changeset |
Node.js 25 became "current" and our CI is now testing it. We now have a failure because "localStorage" is now a thing in Node.js but it's not properly enabled without the arg. So @typescript/vfs, which in the browser detects a "localStorage" and tries to use it (but previously in Node.js wouldn't find it so wouldn't try) finds an implementation that's incomplete. This change activates it, so we don't get the error. But we don't really need it, so this can be wound back when we have a fix in vfs or somewhere else in the stack. Ref: nodejs/node#57666 Ref: microsoft/TypeScript-Website#3449 Ref: microsoft/TypeScript-Website#3450
@yamcodes please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Sigh, the inclusion of the changeset (even though I did that) makes the PR need a CLA due to modifying more than a single line |
Node.js 25 became "current" and our CI is now testing it. We now have a failure because "localStorage" is now a thing in Node.js but it's not properly enabled without the arg. So @typescript/vfs, which in the browser detects a "localStorage" and tries to use it (but previously in Node.js wouldn't find it so wouldn't try) finds an implementation that's incomplete. This change activates it, so we don't get the error. But we don't really need it, so this can be wound back when we have a fix in vfs or somewhere else in the stack. Ref: nodejs/node#57666 Ref: microsoft/TypeScript-Website#3449 Ref: microsoft/TypeScript-Website#3450
The package assumes it's running in a browser environment and directly calls localStorage.getItem("DEBUG"), however:, due to nodejs/node#57666:
Node.js <25:
hasLocalStorage
is false becauselocalStorage
isundefined
, so thelocalStorage
check is skipped entirely ✅Node.js <=25:
hasLocalStorage
is true becauselocalStorage
exists, but it's a mock object that doesn't implement the full API ❌So in Node.js 25,
localStorage
exists butlocalStorage.getItem
is not a function.Node <25, check if
localStorage
existsRun:
node -e "console.log('localStorage exists:', typeof localStorage !== 'undefined'); console.log('localStorage type:', typeof localStorage); console.log('localStorage.getItem type:', typeof localStorage?.getItem);"
Prints:
Node 25, check if
localStorage
existsRun:
node -e "console.log('localStorage exists:', typeof localStorage !== 'undefined'); console.log('localStorage type:', typeof localStorage); console.log('localStorage.getItem type:', typeof localStorage?.getItem);"
Prints:
This PR fixes it.
Closes #3449