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

Add Polyfill based on es-abstract #160

Merged
merged 9 commits into from Oct 8, 2019

Conversation

@pipobscure
Copy link
Collaborator

commented Oct 7, 2019

No description provided.

Copy link
Member

left a comment

All the slot accesses should probably use WeakMaps in a closure, and throw if the instance isn’t in the map, to replicate the brand checks that will be needed?

README.md Show resolved Hide resolved
polyfill/lib/absolute.mjs Outdated Show resolved Hide resolved
polyfill/lib/absolute.mjs Outdated Show resolved Hide resolved
import { SLOT_EPOCHNANOSECONDS } from "./slots.mjs";

export function Absolute(epochNanoSeconds) {
if (!(this instanceof Absolute)) return new Absolute(epochNanoseconds);

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 8, 2019

Member

why not use class syntax for these?

This comment has been minimized.

Copy link
@pipobscure

pipobscure Oct 8, 2019

Author Collaborator

Because most other intrinsic objects can be created/cast while omitting the new. For consistency sake, I wanted the same behaviour. In additions Time(15, 30, 45) without the new actually feels more natural/convenient, especially given that they are immutable objects.

This comment has been minimized.

Copy link
@littledan

littledan Oct 8, 2019

Member

ES6 and later tends to require new for classes, rather than making them callable. I think we should follow this convention in Temporal.

This comment has been minimized.

Copy link
@pipobscure

pipobscure Oct 8, 2019

Author Collaborator

I disagree because of patterns like:

dateTime.with({ day: 5 })
dateTime.with(Date(2019,8,10))
Time(dateTime) // casting to Time
Date(dateTime) // casting to Date

I think this makes things so much more ergonomic. So if you feel strongly, can you open an issue so we can have that discussion properly.

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 8, 2019

Member

That behavior is generally considered legacy; a new issue would be appropriate.

This comment has been minimized.

Copy link
@littledan

littledan Oct 8, 2019

Member

Agreed with @ljharb. Let's go over these sorts of decisions in the Temporal meeting today; I put it on the agenda.

return new Intl.DateTimeFormat(...args).format(this);
};
Date.prototype.toJSON = function toJSON() {
return this.toString();

This comment has been minimized.

Copy link
@ljharb

ljharb Oct 8, 2019

Member

should this observably call toString on the receiver, or should it just === Date.prototype.toString?

@pipobscure pipobscure force-pushed the pipobscure:polyfill branch from 89f12b3 to 899aeb3 Oct 8, 2019
README.md Show resolved Hide resolved
@pipobscure

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 8, 2019

Just to clarify: the point of this polyfill is to almost read like spectext, so as to enable easier writing which will happen next. Because some of the abstracts (especially around timezones) are simple and cheap in C++ bit expensive in JS right now, this chooses clarity over performance.

Once this is done, I will create a version that can be used in production where I can use all the tricks in the book at the cost of clarity.

@pipobscure pipobscure force-pushed the pipobscure:polyfill branch from 481e9eb to 6b23602 Oct 8, 2019
@ryzokuken ryzokuken added this to In progress in Finalize polyfill via automation Oct 8, 2019
@ryzokuken ryzokuken added this to the Stage 3 milestone Oct 8, 2019
@pipobscure pipobscure merged commit c13cfb6 into tc39:main Oct 8, 2019
Finalize polyfill automation moved this from In progress to Done Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.