Skip to content
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

Jump to bottom

MakePrimitiveRef generic so that UNDEFINED_REFERENCE can be Reference<undefined> rather than Reference<unknown> #1481

Merged
merged 1 commit into from
Nov 1, 2023
Merged

MakePrimitiveRef generic so that UNDEFINED_REFERENCE can be Reference<undefined> rather than Reference<unknown> #1481

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions packages/@glimmer/reference/lib/reference.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,8 @@ class ReferenceImpl<T = unknown> implements Reference<T> {
}
}

export function createPrimitiveRef(value: unknown): Reference {
const ref = new ReferenceImpl(UNBOUND);
export function createPrimitiveRef<T = unknown>(value: T): Reference<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don’t think you need = unknown here.

I wonder if you will catch any unexpected/unintended usages if you do T extends string | number | boolean | null | undefined (we may have a type alias for that already somewhere)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don’t think you need = unknown here.

I will try this out in a followup PR!
Since it's so hard to iterate between this PR and ember, I want try as isolated changes at a time as possible

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure. The = unknown is redundant regardless of whether the constraints work out or not. As far as I know it would always have inferred (to unknown in the worst case). For types/interface sometimes you need to say = unknown to be able to reference the type more easily, but I don’t think that’s needed in a function parameter position

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we may have a type alias for that already somewhere

looks like it's just duplicated everywhere -- probably good for better errors, tbh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#1483

const ref = new ReferenceImpl<T>(UNBOUND);

ref.tag = CONSTANT_TAG;
ref.lastValue = value;
Expand All @@ -71,8 +71,8 @@ export function createPrimitiveRef(value: unknown): Reference {

export const UNDEFINED_REFERENCE = createPrimitiveRef(undefined);
export const NULL_REFERENCE = createPrimitiveRef(null);
export const TRUE_REFERENCE = createPrimitiveRef(true);
export const FALSE_REFERENCE = createPrimitiveRef(false);
export const TRUE_REFERENCE = createPrimitiveRef(true as const);
export const FALSE_REFERENCE = createPrimitiveRef(false as const);

export function createConstRef(value: unknown, debugLabel: false | string): Reference {
const ref = new ReferenceImpl(CONSTANT);
Expand Down