GetIntrinsic: remove from PlainDate, add monkeypatching test #105

jens-ox · 2021-12-08T23:22:55Z

Hi all! As discussed in #103, it would be nice to remove GetIntrinsic and just import things.

This PR removes GetIntrinsic (as of writing this only from plainDate.ts in order to make sure that it makes sense before removing it elsewhere).

I also added a test that monkey-patches Duration (used by PlainDate) to make sure that it doesn't affect the behaviour of PlainDate -- not sure if I did it correctly, as I never used monkey-patching before.

As importing something from a file doesn't import from globalThis anyway (at least that's my understanding), monkey-patches done in userland should not the library. If someone with more knowledge of monkey-patching could verify this, that would probably be great.

Patching stuff in userland also seems the main usecase for ES6 Proxies, so maybe this isn't something to worry about anyway?

ptomato · 2021-12-09T20:09:22Z

test/monkeypatch.mjs

+describe('Monkeypatch', () => {
+  describe('PlainDate', () => {
+    it("Monkeypatching Duration doesn't affect PlainDate", () => {
+      Temporal.Duration.prototype.constructor = null;


Probably we also want to check Temporal.Duration = null and delete Temporal.Duration

As the tests are .mjs, imports will be read-only, so JS won't let us do those two things (although monkeypatching the prototype seems to be valid).

justingrant · 2021-12-13T10:12:25Z

HI @jens-ox - I spent some time digging into what would be involved in getting all of Temporal to compile with strictNullChecks: true and strictPropertyInitialization: true, so that you won't be blocked by trying to understand the most confusing parts of the polyfill codebase which are ecmascript.ts and calendar.ts.

The good news is that once those two files are made TS-strict-friendly, the rest of the codebase requires only a handful of changes to compile with strictNullChecks: true and strictPropertyInitialization: true.

The bad news is that those two files are much larger and harder to understand than the rest of Temporal, which will make it really hard for someone new coming to the project.

So I'd suggest that you start with this intrinsics PR as a good first step.

In parallel, I can help out by making needed changes in ecmascript.ts and calendar.ts. The latter also needs to be fully upgraded to TS types (it still uses a lot of any) and needs some refactoring to be maintainable by developers outside the Temporal champions group, so it's a good excuse to get that done.

I should have a PR done for those changes on Monday. This should make it much easier for you to layer the GetIntrinsic work on top.

Sound like an OK plan?

re: monkeypatching, here's some quick use cases that should continue to work: (I believe that they work in today's polyfill, but it's probably good to test these in a real app to make sure they actually work on top of v0.3.0 of the polyfill)

import { Temporal } from '@js-temporal/polyfill';

// now monkeypatch a Temporal method
Temporal.PlainDate.prototype.equals = () => { return 'not equal!'; }
d = Temporal.PlainDate.from('2020-01-01')

// calling that method should run the patched code
d.equals('2020-01-01')
// => 'not equal!'

// If another Temporal type creates an instance of that method, it should also run the patched code
Temporal.ZonedDateTime.from('2020-01-01[America/Los_Angeles]').toPlainDate().equals('2020-01-01')
// => 'not equal!'

// patching again will change behavior of *existing* instances
Temporal.PlainDate.prototype.equals = () => { return 'actually, equal!'; }
d.equals('2020-01-01')
// => 'actually, equal!'

// also works for static methods
Temporal.PlainDate.compare = (d1, d2) => { return 42; }
Temporal.PlainDate.compare('2020-01-01', '2020-01-01');
// => 42

// can also replace entire types
Temporal.PlainDate = class { constructor() { this.year = 1999 }}
new Temporal.PlainDate().year
// => 1999

// however, those replaced types won't be used by Temporal internal code
// when creating new instances.  It will use intrinsic types.  
Temporal.ZonedDateTime.from('2020-01-01[America/Los_Angeles]').toPlainDate().year
// => 2020

// but those intrinsic types still retain the patches to its prototype
Temporal.ZonedDateTime.from('2020-01-01[America/Los_Angeles]').toPlainDate().equals('2020-01-01')
// => 'actually, equal!'

@ptomato (or @gibson42 who seems to know more about monkeypatching than anyone)

First, are the above cases how monkeypatching should work?
Second, what are the pitfalls or things that can go wrong when trying to enable monkeypatching, that @jens-ox should be watching out for?

12wrigja · 2021-12-13T21:37:03Z

@gibson042 (as I think Justin typo'd above)

ptomato · 2021-12-13T21:47:31Z

First, are the above cases how monkeypatching should work?

I believe so. While checking whether Temporal.PlainDate = ... should be possible, I realized we don't actually specify the property descriptor of Temporal.___, but I assume that those need to be writable=true and configurable=true so that monkeypatching remains possible. (And we actually have test262 tests to verify this already.) I'll open an issue for this in proposal-temporal.

Second, what are the pitfalls or things that can go wrong when trying to enable monkeypatching, that @jens-ox should be watching out for?

The main thing I was concerned about when switching from GetIntrinsic to internal imports was your second-to-last example (that instances created internally in Temporal might be instances of the "public" replaced type, which would be bad.)

…ve-get-intrinsic

jens-ox · 2021-12-14T21:35:11Z

Hi all, I fully removed intrinsicclass.ts. The failing test262 tests also seem to fail on main.

I'll try to add the monkeypatches from above, but as the tests are .mjs files, the imports are read-only.

jens-ox · 2021-12-14T21:46:33Z

There was also some funky stuff going on with calendar initialization in ecmascript.ts that made TS angry, so I fixed that (moved the initialization to a ternary as suggested by an inline TODO)

justingrant · 2021-12-14T23:08:56Z

lib/intrinsicclass.ts

-    configurable: true
-  });
-  if (DEBUG) {
-    Object.defineProperty(Class.prototype, Symbol.for('nodejs.util.inspect.custom'), {


Is this functionality (which AFAIK provides a way for Node to show friendlier output in the debugger) moved somewhere else? If not, then it probably should be, unless for some reason there's a better way to handle this.

@ptomato is this currently in use somewhere (as you added this)? Seems like a few other libraries (e.g. Moment.js) use this, but doesn't seem to be super common. If it's needed I would create a debug.ts, add the method there and attach it to the classes explicitly.

This is mainly useful when debugging or testing things out interactively. I care mostly that it is present in proposal-temporal, I think it's probably useful here as well, but I'd leave it up to the people who maintain this polyfill more actively than me.

justingrant · 2021-12-15T06:45:26Z

Hi all, I fully removed intrinsicclass.ts.

Cool! The resulting code is much more readable. Very nice!

The failing test262 tests also seem to fail on main.

The one failure on main was an "unexpected pass" because its PR was merged but we didn't update expected-failures.txt. If you rebase on top of #110 then this failure will go away. There should be no other failures on main. If you're seeing more than one failure on main, could you git pull from upstream and see if that resolves any other failures in your local main?

The other failure does not occur on main, and it's a concerning one:

FAIL built-ins/Temporal/Instant/prototype/builtin.js (strict mode)

What's odd is that when I checked out your PR branch and ran this test in the debugger, it didn't fail. Not sure why. Perhaps because I'm not correctly picking up any changes to Test262 tests that you made (did you make any?)

Could you try rebasing your PR on top of #110 (which updates Test262 to latest) and see what, if any, failures remain?

justingrant · 2021-12-15T17:13:12Z

Could you try rebasing your PR on top of #110 (which updates Test262 to latest) and see what, if any, failures remain?

Update: #110 was just merged so you can just rebase to main.

Previously, GetIntrinsic returned a type that could be `undefined`. That's fixed in this commit. Note that intrinsics.ts may be removed completely in js-temporal#105; if that PR lands first then we'll remove this commit.

…ve-get-intrinsic

jens-ox · 2021-12-17T10:26:07Z

Branch is rebased 👍

jens-ox · 2021-12-18T17:52:23Z

I did some testing today, monkeypatching things will still work in CommonJS. Using the module import, this will not work, as the imports are read-only.

justingrant · 2021-12-18T18:43:03Z

I did some testing today, monkeypatching things will still work in CommonJS. Using the module import, this will not work, as the imports are read-only.

@jens-ox - Do all the cases in #105 (comment) work when the polyfill is imported in an ESM app (other than modifying properties on exported objects, as noted above) or require-d in a CJS app?

@ptomato - I noticed a subtle issue in monkeypatch behavior: should static methods on Temporal types be patchable? If my understanding of export is correct, then the following polyfill code would not allow patching of static methods because the properties of imported exports are read-only:

// plaindate.mjs
export class PlainDate { ... }

// temporal.mjs
export { PlainDate } from './plaindate.mjs';

But I don't see anything in the spec that describes whether static methods like PlainDate.compare should be writeable or read-only. (Or configurable, or...) See tc39/proposal-temporal#1978 (comment).

justingrant · 2021-12-20T19:47:29Z

FYI @jens-ox - most of the other maintainers of this polyfill are out for the holidays, so it's unlikely that you'll get more feedback on this PR until January. Just wanted to let you know we weren't ignoring you! 😄

jens-ox · 2021-12-20T22:16:51Z

That's fine @justingrant 😊 I'm currently digging through the code - if it's ok I'm going to create a discussion post with some questions regarding the codebase.

justingrant · 2021-12-20T22:42:06Z

I'm currently digging through the code - if it's ok I'm going to create a discussion post with some questions regarding the codebase.

Sounds good. I'll be around over the holidays so can answer questions.

In parallel, I can help out by making needed changes in ecmascript.ts and calendar.ts. The latter also needs to be fully upgraded to TS types (it still uses a lot of any) and needs some refactoring to be maintainable by developers outside the Temporal champions group, so it's a good excuse to get that done.

I should have a PR done for those changes on Monday. This should make it much easier for you to layer the GetIntrinsic work on top.

BTW, this work is here: #109. It turned out that after converting those two files to be strictNullChecks-compatible, there was almost no other changes required so I just included those in the PR too. So there's no need for another PR to enable strictNullChecks project-wide.

justingrant · 2021-12-20T22:54:41Z

there's no need for another PR to enable strictNullChecks project-wide.

To be clear, this PR #105 is still good (assuming we can figure out the monkeypatching stuff discussed above) but we can close #103 because it's covered by #109 along with a few recent already-merged PRs (#104, #106, #108, #110, #111, #112) that came out of the investigation of #109.

jens-ox · 2021-12-20T22:57:35Z

Sounds great! Regarding monkeypatching - this seems to work on my end, and the monkeypatching capabilities testable in a .mjs work as expected. When importing the CommonJS bundle and doing some monkeypatching, this also works as expected. Automating this as a proper test is probably not really possible, as all tests are modules.

jens-ox · 2021-12-20T22:59:03Z

Here is the content of the test.cjs I used:

/* eslint-disable no-console */
const { Temporal } = require('./dist/index.cjs');

Temporal.PlainDate.prototype.equals = () => {
  return 'not equal!';
};
const d = Temporal.PlainDate.from('2020-01-01');

// calling that method should run the patched code
console.log(d.equals('2020-01-01'));
// => 'not equal!'

// If another Temporal type creates an instance of that method, it should also run the patched code
Temporal.ZonedDateTime.from('2020-01-01[America/Los_Angeles]').toPlainDate().equals('2020-01-01');
// => 'not equal!'

// patching again will change behavior of *existing* instances
Temporal.PlainDate.prototype.equals = () => {
  return 'actually, equal!';
};
console.log(d.equals('2020-01-01'));

// also works for static methods
Temporal.PlainDate.compare = (d1, d2) => {
  return 42;
};
console.log(Temporal.PlainDate.compare('2020-01-01', '2020-01-01'));
// => 42

// can also replace entire types
Temporal.PlainDate = class {
  constructor() {
    this.year = 1999;
  }
};
console.log(new Temporal.PlainDate().year);
// => 1999

// however, those replaced types won't be used by Temporal internal code
// when creating new instances.  It will use intrinsic types.
console.log(Temporal.ZonedDateTime.from('2020-01-01[America/Los_Angeles]').toPlainDate().year);
// => 2020

// but those intrinsic types still retain the patches to its prototype
console.log(Temporal.ZonedDateTime.from('2020-01-01[America/Los_Angeles]').toPlainDate().equals('2020-01-01'));
// => 'actually, equal!'

ptomato · 2022-01-07T18:49:11Z

I took a look at this, not in detail because most of it is searching and replacing GetIntrinsic and I trust that the TypeScript compiler and the tests are ensuring that that was done correctly. Is there any part of this PR in particular you'd like me to pay attention to?

Previously, GetIntrinsic returned a type that could be `undefined`. That's fixed in this commit. Note that intrinsics.ts may be removed completely in #105; if that PR lands first then we'll remove this commit.

justingrant · 2022-01-13T00:11:20Z

Hi @jens-ox - We're all back from the holiday break and ready to review this. There's been quite a few PRs merged recently so now this PR has a lot of merge conflicts. Could you rebase it? Thanks!

justingrant · 2022-02-04T02:41:26Z

Hi @jens-ox - We're all back from the holiday break and ready to review this. There's been quite a few PRs merged recently so now this PR has a lot of merge conflicts. Could you rebase it? Thanks!

@jens-ox, are you still interested in completing this PR? Let us know if someone else should pick it up. Thanks!

jens-ox · 2022-02-04T08:55:38Z

Hi @justingrant, I'll pick this up this weekend 👍

justingrant · 2022-02-18T01:06:59Z

Hi @justingrant, I'll pick this up this weekend 👍

Hi @jens-ox - any luck getting back to this? Would be good to wrap this up before more merge conflicts are introduced.

…ve-get-intrinsic

jens-ox · 2022-07-09T13:05:39Z

Hi @justingrant! Sorry, I got a new job and completely forgot about my open source things. I updated the PR - but feel free to close this if this is not relevant anymore.

remove getIntrinsic from plaindate, add monkeypatching test

71239c8

jens-ox mentioned this pull request Dec 8, 2021

RFC: Fix null-check stuff #103

Closed

ptomato reviewed Dec 9, 2021

View reviewed changes

justingrant mentioned this pull request Dec 13, 2021

Compile with strictNullChecks/strictPropertyInitialization (focus on calendar.ts and ecmascript.ts) #109

Merged

jens-ox added 3 commits December 14, 2021 21:30

Merge branch 'main' of github.com:jens-ox/temporal-polyfill into remo…

f114d6c

…ve-get-intrinsic

wip

3dd5bef

merging with changes on main

f2684b5

jens-ox marked this pull request as ready for review December 14, 2021 21:23

fix merge error, update test262

4c126eb

justingrant reviewed Dec 14, 2021

View reviewed changes

Merge branch 'main' of github.com:jens-ox/temporal-polyfill into remo…

fe81b40

…ve-get-intrinsic

update tests

34c3049

12wrigja mentioned this pull request Feb 18, 2022

Release 0.4.0 #139

Closed

jens-ox added 3 commits July 9, 2022 15:01

Merge branch 'main' of github.com:jens-ox/temporal-polyfill into remo…

c4753f4

…ve-get-intrinsic

remove unnecessary defaults from package.json

516b4d3

remove unnecessary defaults from tsconfig.json

1c4fc3b

12wrigja mentioned this pull request Jul 26, 2022

Implements globalThis slots #175

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GetIntrinsic: remove from PlainDate, add monkeypatching test #105

GetIntrinsic: remove from PlainDate, add monkeypatching test #105

jens-ox commented Dec 8, 2021

ptomato Dec 9, 2021

jens-ox Dec 10, 2021

justingrant commented Dec 13, 2021

12wrigja commented Dec 13, 2021

ptomato commented Dec 13, 2021

jens-ox commented Dec 14, 2021

jens-ox commented Dec 14, 2021

justingrant Dec 14, 2021

jens-ox Dec 17, 2021

ptomato Jan 7, 2022

justingrant commented Dec 15, 2021

justingrant commented Dec 15, 2021

jens-ox commented Dec 17, 2021 •

edited

Loading

jens-ox commented Dec 18, 2021

justingrant commented Dec 18, 2021

justingrant commented Dec 20, 2021

jens-ox commented Dec 20, 2021

justingrant commented Dec 20, 2021

justingrant commented Dec 20, 2021

jens-ox commented Dec 20, 2021

jens-ox commented Dec 20, 2021

ptomato commented Jan 7, 2022

justingrant commented Jan 13, 2022

justingrant commented Feb 4, 2022

jens-ox commented Feb 4, 2022

justingrant commented Feb 18, 2022

jens-ox commented Jul 9, 2022

GetIntrinsic: remove from PlainDate, add monkeypatching test #105

Are you sure you want to change the base?

GetIntrinsic: remove from PlainDate, add monkeypatching test #105

Conversation

jens-ox commented Dec 8, 2021

ptomato Dec 9, 2021

Choose a reason for hiding this comment

jens-ox Dec 10, 2021

Choose a reason for hiding this comment

justingrant commented Dec 13, 2021

12wrigja commented Dec 13, 2021

ptomato commented Dec 13, 2021

jens-ox commented Dec 14, 2021

jens-ox commented Dec 14, 2021

justingrant Dec 14, 2021

Choose a reason for hiding this comment

jens-ox Dec 17, 2021

Choose a reason for hiding this comment

ptomato Jan 7, 2022

Choose a reason for hiding this comment

justingrant commented Dec 15, 2021

justingrant commented Dec 15, 2021

jens-ox commented Dec 17, 2021 • edited Loading

jens-ox commented Dec 18, 2021

justingrant commented Dec 18, 2021

justingrant commented Dec 20, 2021

jens-ox commented Dec 20, 2021

justingrant commented Dec 20, 2021

justingrant commented Dec 20, 2021

jens-ox commented Dec 20, 2021

jens-ox commented Dec 20, 2021

ptomato commented Jan 7, 2022

justingrant commented Jan 13, 2022

justingrant commented Feb 4, 2022

jens-ox commented Feb 4, 2022

justingrant commented Feb 18, 2022

jens-ox commented Jul 9, 2022

jens-ox commented Dec 17, 2021 •

edited

Loading