Skip to content

RFC: Fix null-check stuff #103

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

Closed
wants to merge 3 commits into from
Closed

RFC: Fix null-check stuff #103

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52a048a
fix ts issues in plaindate
jens-ox Dec 7, 2021
ad911d8
remove getIntrinsic
jens-ox Dec 7, 2021
5aec4ed
Merge branch 'main' of github.com:jens-ox/temporal-polyfill into ts-i…
jens-ox Dec 7, 2021
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
4 changes: 2 additions & 2 deletions lib/calendar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export class Calendar implements Temporal.Calendar {
return ES.ToString(this);
}
dateFromFields(
fields: Params['dateFromFields'][0],
fields?: Params['dateFromFields'][0],
Copy link
Contributor

Choose a reason for hiding this comment

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

fields should never be undefined.

Also, if undefined were acceptable, then for any public API (like this one) it's intentional to leave it as Params... because that enforces alignment between the implementation and index.d.ts. So let's imagine that undefined were OK for this API, then the right fix would be to change index.d.ts, not here.

optionsParam: Params['dateFromFields'][1] = undefined
): Return['dateFromFields'] {
if (!ES.IsTemporalCalendar(this)) throw new TypeError('invalid receiver');
Expand Down Expand Up @@ -178,7 +178,7 @@ export class Calendar implements Temporal.Calendar {
if (!ES.IsTemporalCalendar(this)) throw new TypeError('invalid receiver');
const date = ES.ToTemporalDate(dateParam);
const duration = ES.ToTemporalDuration(durationParam);
const options = ES.GetOptionsObject(optionsParam);
const options = ES.GetOptionsObject<Temporal.ArithmeticOptions>(optionsParam);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this explicit type parameter needed? Can't the type be inferred by TS from the type of options ?

const overflow = ES.ToTemporalOverflow(options);
const { days } = ES.BalanceDuration(
GetSlot(duration, DAYS),
Expand Down
22 changes: 11 additions & 11 deletions lib/ecmascript.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1069,8 +1069,8 @@ export function ToPartialRecord<B extends AnyTemporalLikeType>(
}

export function PrepareTemporalFields<B extends AnyTemporalLikeType>(
bag: B,
fields: ReadonlyArray<FieldRecord<B>>
bag?: B,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably the wrong solution. See tc39/proposal-temporal#1963.

fields: ReadonlyArray<FieldRecord<B>> = []
): Required<B> | undefined {
if (!IsObject(bag)) return undefined;
const result = {} as B;
Expand Down Expand Up @@ -1109,8 +1109,8 @@ export function PrepareTemporalFields<B extends AnyTemporalLikeType>(

// field access in the following operations is intentionally alphabetical
export function ToTemporalDateFields(
bag: Temporal.PlainDateLike,
fieldNames: readonly (keyof Temporal.PlainDateLike)[]
bag?: Temporal.PlainDateLike,
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above.

fieldNames: readonly (keyof Temporal.PlainDateLike)[] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing runtime behavior like this is probably a bad idea because it risks deviating from the spec and from the non-prod polyfill behavior in proposal-temporal. Probably better to look upstream to see if undefined is actually valid here or not.

) {
const entries: [keyof Temporal.PlainDateLike, 0 | undefined][] = [
['day', undefined],
Expand Down Expand Up @@ -1899,8 +1899,8 @@ export function CalendarFields<F extends Iterable<string>>(calendar: Temporal.Ca

export function CalendarMergeFields(
calendar: Temporal.CalendarProtocol,
fields: Record<string, unknown>,
additionalFields: Record<string, unknown>
fields?: Record<string, unknown>,
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK both of these fields are not optional. (Although should check upstream about the second one-- not sure)

additionalFields?: Record<string, unknown>
) {
const calMergeFields = calendar.mergeFields;
if (!calMergeFields) {
Expand Down Expand Up @@ -2102,7 +2102,7 @@ export function ConsolidateCalendars(one: Temporal.CalendarProtocol, two: Tempor

export function DateFromFields(
calendar: Temporal.CalendarProtocol,
fields: CalendarProtocolParams['dateFromFields'][0],
fields?: CalendarProtocolParams['dateFromFields'][0],
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK this is not optional.

options?: Partial<CalendarProtocolParams['dateFromFields'][1]>
) {
const result = calendar.dateFromFields(fields, options);
Expand All @@ -2112,7 +2112,7 @@ export function DateFromFields(

export function YearMonthFromFields(
calendar: Temporal.CalendarProtocol,
fields: CalendarProtocolParams['yearMonthFromFields'][0],
fields?: CalendarProtocolParams['yearMonthFromFields'][0],
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK this is not optional.

options?: CalendarProtocolParams['yearMonthFromFields'][1]
) {
const result = calendar.yearMonthFromFields(fields, options);
Expand All @@ -2122,7 +2122,7 @@ export function YearMonthFromFields(

export function MonthDayFromFields(
calendar: Temporal.CalendarProtocol,
fields: CalendarProtocolParams['monthDayFromFields'][0],
fields?: CalendarProtocolParams['monthDayFromFields'][0],
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK this is not optional.

options?: CalendarProtocolParams['monthDayFromFields'][1]
) {
const result = calendar.monthDayFromFields(fields, options);
Expand Down Expand Up @@ -4666,7 +4666,7 @@ export function RoundDuration(
nanosecondsParam: number,
increment: number,
unit: Temporal.DateTimeUnit,
roundingMode: Temporal.RoundingMode,
roundingMode: Temporal.RoundingMode = 'trunc',
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, defaults are assigned upstream, not in default arguments like this. So I think this change is probably wrong.

relativeToParam: ReturnType<typeof ToRelativeTemporalObject> = undefined
) {
let years = yearsParam;
Expand Down Expand Up @@ -4996,7 +4996,7 @@ export function ComparisonResult(value: number) {
return value < 0 ? -1 : value > 0 ? 1 : (value as 0);
}

export function GetOptionsObject<T>(options: T) {
export function GetOptionsObject<T>(options: T | undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why this change is needed? If undefined is missing from the caller's type, then the fix is probably needed in the caller, not here. By adding undefined here we're probably hiding a type bug in the caller.

if (options === undefined) return ObjectCreate(null) as T;
if (IsObject(options) && options !== null) return options;
throw new TypeError(`Options parameter must be an object, not ${options === null ? 'null' : `${typeof options}`}`);
Expand Down
11 changes: 5 additions & 6 deletions lib/plaindate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as ES from './ecmascript';
import { GetIntrinsic, MakeIntrinsicClass } from './intrinsicclass';
import { MakeIntrinsicClass } from './intrinsicclass';
import {
ISO_YEAR,
ISO_MONTH,
Expand Down Expand Up @@ -188,8 +188,8 @@ export class PlainDate implements Temporal.PlainDate {
this
));

const Duration = GetIntrinsic('%Temporal.Duration%');
return new Duration(years, months, weeks, days, 0, 0, 0, 0, 0, 0);
// const Duration = GetIntrinsic('%Temporal.Duration%')!;
return new Temporal.Duration(years, months, weeks, days, 0, 0, 0, 0, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This we can't do, because it would break if user code monkey-patched globalThis.Temporal.Duration. (That's the reason for the GetIntrinsic mechanism appearing all over the place, so that we can continue to use the original values internally even if they are replaced in user code.) Sorry, this should've been documented somewhere more clearly! Probably in intrinsic.ts.

I think the best solution here (and elsewhere in the file) would be to type GetIntrinsic such that TypeScript knows that it returns Temporal.Duration for an argument of %Temporal.Duration%. Unfortunately I don't immediately have an idea of how to do that properly 😄

Copy link
Author

Choose a reason for hiding this comment

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

Could this be avoided by just doing import { Duration } from './duration'? Or am I missing something? 😅
Makes sense otherwise, if importing is not sensible here I'll try to teach TypeScript how GetIntrinsic works 😊

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can import as it causes circular value dependencies between the different files? If that would work that could be one potential fix.

If we need to stick with GetIntrinsic it should already be typed such that a key of %Temporal.Duration% returns typeof Temporal.Duration, but what probably needs to change is that the return type for GetIntrinsic needs to be NonNullable<typeof INTRINSICS[key]> here.

Copy link
Author

Choose a reason for hiding this comment

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

Circular dependencies should be fine as there's already ecmascript -> calendar -> ecmascript :D
If just importing is an option that might be cleaner, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

How does TypeScript usually handle circular module dependencies?

}
since(otherParam: Params['since'][0], optionsParam: Params['since'][1] = undefined): Return['since'] {
if (!ES.IsTemporalDate(this)) throw new TypeError('invalid receiver');
Expand All @@ -212,9 +212,8 @@ export class PlainDate implements Temporal.PlainDate {

const untilOptions = { ...options, largestUnit };
let { years, months, weeks, days } = ES.CalendarDateUntil(calendar, this, other, untilOptions);
const Duration = GetIntrinsic('%Temporal.Duration%');
if (smallestUnit === 'day' && roundingIncrement === 1) {
return new Duration(-years, -months, -weeks, -days, 0, 0, 0, 0, 0, 0);
return new Temporal.Duration(-years, -months, -weeks, -days, 0, 0, 0, 0, 0, 0);
}
({ years, months, weeks, days } = ES.RoundDuration(
years,
Expand All @@ -233,7 +232,7 @@ export class PlainDate implements Temporal.PlainDate {
this
));

return new Duration(-years, -months, -weeks, -days, 0, 0, 0, 0, 0, 0);
return new Temporal.Duration(-years, -months, -weeks, -days, 0, 0, 0, 0, 0, 0);
}
equals(otherParam: Params['equals'][0]): Return['equals'] {
if (!ES.IsTemporalDate(this)) throw new TypeError('invalid receiver');
Expand Down
3 changes: 2 additions & 1 deletion lib/slots.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,10 @@ export function HasSlot(container: unknown, ...ids: (keyof Slots)[]): boolean {
return !!myslots && ids.reduce((all: boolean, id) => all && id in myslots, true);
}
export function GetSlot<KeyT extends keyof Slots>(
container: Slots[typeof id]['usedBy'],
container: Slots[typeof id]['usedBy'] | undefined,
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 it signifies a bug in the polyfill if we end up calling GetSlot(undefined, ...) so I'd be happy if TS would complain at compile time rather than resorting to a runtime error. I wonder if there's any way to do that?

Copy link
Author

Choose a reason for hiding this comment

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

In this case it can be undefined if an incorrect disambiguation is given to BuiltinTimeZoneGetInstantFor, so maybe it would be best to just throw?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can tighten the types in BuiltinTimeZoneGetInstantFor to ensure at compile time that no incorrect disambiguation can be given there? Or otherwise throw earlier if that happens?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like there may be a legit issue with DisambiguatePossibleInstants because the spec says there should be an assertion that the disambiguation value is valid but the polyfill lacks this assertion. We could add a line at the end of that method e.g.

    throw new Error(`assertion failed: invalid disambiguation value ${disambiguation}`);

id: KeyT
): Slots[KeyT]['value'] {
if (container === undefined) throw new TypeError('Missing container');
const value = GetSlots(container)[id];
if (value === undefined) throw new TypeError(`Missing internal slot ${id}`);
return value;
Expand Down