-
Notifications
You must be signed in to change notification settings - Fork 6
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
tweaks for satisfying typechecker #9
Conversation
@@ -22,6 +22,29 @@ const HAS_PENDING_UPDATE = 1 << 0; | |||
const HAS_HOOK_STATE = 1 << 1; | |||
const HAS_COMPUTEDS = 1 << 2; | |||
|
|||
const enum OptionsTypes { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I goofed on my previous pr (#7) and put these types after the code that uses them, which TypeScript doesn't like.
@@ -12,7 +12,7 @@ const freeModule = freeExports && typeof module === 'object' && module !== null | |||
const moduleExports = freeModule && freeModule.exports === freeExports | |||
|
|||
/** Detect free variable `process` from Node.js. */ | |||
const freeProcess = moduleExports && freeGlobal.process | |||
const freeProcess = moduleExports && freeGlobal && freeGlobal.process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
freeGlobal
is inferred to have a type of false | typeof globalThis
, so we need to ensure that freeGlobal
isn't false
before checking its properties.
@@ -660,4 +660,4 @@ function effect(compute: () => unknown): () => void { | |||
return effect._dispose.bind(effect); | |||
} | |||
|
|||
export { signal, computed, effect, batch, Signal, ReadonlySignal }; | |||
export { signal, computed, effect, batch, Signal, type ReadonlySignal }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the identifiers exported on this line are both types and values, but ReadonlySignal
is just a type. The type
annotation helps bundlers deal with this correctly (read more).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I didn't know about this before. A bit more from ChatGPT:
Some third-party JavaScript bundlers like webpack and Rollup might not understand TypeScript's semantics around types, since they don't exist in JavaScript. They might see an import statement and assume it's importing a value that needs to be included in the bundle, even if it's actually only importing a type that gets erased during TypeScript's compilation step.
This can lead to unnecessarily larger bundles, or even errors if a module that only exports types is not included in the bundle because the bundler assumes it needs to be there at runtime.
By using import type or export type, you're making it explicit to both TypeScript and any tooling that understands TypeScript that these imports and exports are type-only, and so they don't need to be included in the JavaScript output. This can help improve bundle size and reduce potential errors.
The TypeScript team actually recommends using import type and export type when you're working in a codebase that also gets built with a bundler, just to be on the safe side and to make your intent clearer to other developers.
These changes satisfy the
tsc
typechecker assuming that the isolatedModules flag is enabled. This flag is recommended when using TypeScript alongside a bundler such as webpack or esbuild.The changes should have no impact on the behavior of the compiled JS. See inline for specific comments on why certain changes were necessary.