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.

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

Change polyfill back to class #163

Merged
merged 8 commits into from Oct 10, 2019

Conversation

@pipobscure
Copy link
Collaborator

commented Oct 9, 2019

After the working group discussion yesterday, I've migrated back to class meaning temporal objects require new to create.

I have also added a set of casting functions named after the thing they are casting to but lowercase. Let's see and play with this if it feels ergonomic. If it does, then we can ask for more committee input.

@pipobscure pipobscure force-pushed the pipobscure:class branch from 27640ab to 1993812 Oct 9, 2019
@pipobscure pipobscure force-pushed the pipobscure:class branch from 1993812 to 3d42b47 Oct 9, 2019
polyfill/lib/absolute.mjs Outdated Show resolved Hide resolved
polyfill/lib/absolute.mjs Show resolved Hide resolved
polyfill/lib/casts.mjs Outdated Show resolved Hide resolved
@pipobscure

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 9, 2019

I've made sure this can run with a simple babel transform (i.e. no additional polyfills on node 0.12 / v8 3.28.71.20).

This is pretty much the first version of v8 that has the Intl.DateFormat API which is essential (i.e. no polyfill without it).

I've also optimised the code a bit in regards to reusing Intl.DateFormat instances. So this should be productionisable. Of course for that to be fully true, a set of tests would be good. (Next Step)

Copy link
Member

left a comment

Some initial comments. I have not done a full review yet.

polyfill/lib/casts.mjs Outdated Show resolved Hide resolved
polyfill/lib/absolute.mjs Outdated Show resolved Hide resolved
polyfill/lib/absolute.mjs Outdated Show resolved Hide resolved
polyfill/lib/absolute.mjs Show resolved Hide resolved
polyfill/lib/yearmonth.mjs Show resolved Hide resolved
polyfill/lib/absolute.mjs Outdated Show resolved Hide resolved
polyfill/lib/timezone.mjs Show resolved Hide resolved
polyfill/lib/yearmonth.mjs Show resolved Hide resolved
polyfill/lib/casts.mjs Show resolved Hide resolved
polyfill/lib/casts.mjs Show resolved Hide resolved
@pipobscure

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 10, 2019

Thanks @littledan this is really helpful! I'll be working to address these now.

@pipobscure

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 10, 2019

I've closed all the comments that I've addressed by just doing as requested. (using these comments like a todo list).

The few that are open need more conversation on which way to go. I have made the arguments via comment, but will go with whatever @littledan / @ljharb suggest on these.

@pipobscure

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 10, 2019

I'm merging this now since I think the API is fine and the remaining comments are just about detail functonality

I've opened issues for the remaining points #165 #166 #167

@pipobscure pipobscure merged commit a8bd800 into tc39:main Oct 10, 2019
return GetSlot(this, DAY);
}
get dayOfWeek() {
return ES.DayOfWeek(GetSlot(this, THIS).year, GetSlot(this, THIS).month, GetSlot(this, DAY));

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Oct 11, 2019

Collaborator

What's THIS?

case 'balance':
({ year, month, day } = ES.BalanceDate(year, month, day));
break;
default:

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Oct 11, 2019

Collaborator

How do we want to handle unrecognized disambiguation values? I'd suggest throwing, unless there's prior art for "treat as the default".

return 0;
}
}
Date.prototype.toJSON = Date.prototype.toString;

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Oct 11, 2019

Collaborator

Is there prior art for making this the same function object?

@@ -23,8 +25,11 @@ const INTRINSICS = {
'%Temporal.Absolute%': Temporalabsolute,
'%Temporal.Duration%': TemporalDuration
};
for (let name in Cast) {
if (Object.prototype.hasOwnProperty.call(Cast, name)) INTRINSICS[`%Temporal.${name}%`] = Cast[name];
}

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Oct 11, 2019

Collaborator

Do you really want %Temporal.date% and %Temporal.Date% intrinsics that do significantly different things?

const { hour, minute, second, millisecond, microsecond, nanosecond } = time(arg);
return new Time(hour, minute, second, millisecond, microsecond, nanosecond);
}
}

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Oct 11, 2019

Collaborator

Some of these have RangeErrors at the end, some don't.


export const offset = /([+-][01]?[0-9]):?([0-5][0-9])/;
export const duration = /P(?:(\d+)Y)?(?:(\d+)M)?(?:(\d+)D)?(?:T(?:(\d+)H)?(?:(\d+)M)?(?:(\d+(?:\.\d+)?)S)?)?/;

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Oct 11, 2019

Collaborator

Does this need ^$ as well?

}
plus(durationLike = {}, disambiguation = 'constrain') {
const duration = ES.GetIntrinsic('%Temporal.duration%')(durationLike);
let { year, month, day } = this;

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Oct 11, 2019

Collaborator

Should probably use GetSlot.

let { year, month, day } = this;
let { years, months, days, hours, minutes, seconds, milliseconds, microseconds, nanoseconds } = duration;
if (hours || minutes || seconds || milliseconds || microseconds || nanoseconds)
throw new RangeError('invalid duration');

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Oct 11, 2019

Collaborator

In the case where we get a bag of options, it seems weird to me to grab all these properties and then complain when they're non-zero.

In the string case, do we even want to allow, e.g., P1YT0H here?

I feel like it would maybe make more sense to have a separate conversion algorithm for date durations.

year = ES.ToInteger(year);
month = ES.ToInteger(month);
day = ES.ToInteger(day);
switch (disambiguation) {

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Oct 11, 2019

Collaborator

This will need a ToString, I think.

return ES.LeapYear(GetSlot(this, YEAR));
}
with(dateLike = {}, disambiguation = 'constrain') {
const { year = GetSlot(this, YEAR), month = GetSlot(this, MONTH), day = GetSlot(this, DAY) } = dateTimeLike;

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Oct 11, 2019

Collaborator

dateTimeLike is not defined

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.