Check whether this polyfill runs afoul of Dual Package hazards

[12wrigja]

Context: https://nodejs.org/api/packages.html#dual-package-hazard

I was pondering this in the context of #155, specifically whether if two versions of the polyfill sources (one with jsbi, and one with bigint) were to be loaded into the page whether they would correctly interoperate with each other, at the cost of extreme code-size regression (two copies of Temporal srcs, potentially two copies of JSBI!).

My guess is that no, currently they would not, primarily because an object from one instance would not satisfy the slot-based checks when passed to the other:

temporal-polyfill/lib/slots.ts

Line 153 in 56df93f

const slots = new WeakMap();

Setting aside the code-size problems, if it were possible to at least have the results be correct when interoperating I would consider publishing a second package that contains a pre-"transpiled" bundle that does not contain JSBI.

[12wrigja]

If/when we get to a future where Temporal has shipped presumably this is not a problem because each version of the package will simply provide a reference to the native Temporal instance.

[tars0x9752]

an object from one instance would not satisfy the slot-based checks

This sounds right if some kind of slots related things passed to the other. But is "passed to the other" possible? The slots itself looks rely on "module-local state" but I think each slots (I mean "slots containers" or I should say weakmap keys) are isolated between instances and they look quite private. So I haven't figured out how to reproduce this yet.

I just tried a little bit but at least it didn't happen while simply importing two different versions.

import { Temporal as Temporal1 } from 'temporal-0.3.0'
import { Temporal as Temporal2 } from 'temporal-0.4.1'

const a = Temporal1.Now.instant()
const b = Temporal2.Now.instant()
const c = Temporal1.Now.instant()
const d = Temporal2.Now.instant()

console.log('a', a.epochSeconds) // no errors are thrown.
console.log('b', b.epochSeconds) // no errors are thrown.
console.log('c', c.epochSeconds) // no errors are thrown.
console.log('d', d.epochSeconds) // no errors are thrown.

[ptomato]

What happens if you do something like

const a = Temporal1.Now.instant();
const round = Temporal2.Instant.prototype.round;
round.call(a, 'second');

[tars0x9752]

Thanks! It threw the TypeError: invalid receiver that is exactly what I was looking for! So the hazard is definitely reproducible, if multiple different builds/versions are imported.

import { Temporal as Temporal1 } from "temporal-0.3.0";
import { Temporal as Temporal2 } from "temporal-0.4.1";

const instantOfTemporal1 = Temporal1.Now.instant();
const instantOfTemporal2 = Temporal2.Now.instant();

const roundOfTemporal1 = Temporal1.Instant.prototype.round;
const roundOfTemporal2 = Temporal2.Instant.prototype.round;

roundOfTemporal1.call(instantOfTemporal1, "second"); // no errors are thrown
roundOfTemporal2.call(instantOfTemporal2, "second"); // no errors are thrown
roundOfTemporal2.call(instantOfTemporal1, "second"); // TypeError: invalid receiver

[tars0x9752]

So, it turns out that dual package hazards can occur if CommonJS and ESM get mixed. For example:

// lib-a (mjs)
export function getDurationInHours(zdtLeft, zdtRight) {
  return zdtRight.since(zdtLeft).total({ unit: "hour" });
}

// lib-b (cjs)
const { Temporal } = require("@js-temporal/polyfill");

function createZDT(year, month, day) {
  return Temporal.ZonedDateTime.from({
    timeZone: "Asia/Tokyo",
    year,
    month,
    day,
  });
}

module.exports = { createZDT };

// main.mjs
import { Temporal } from "@js-temporal/polyfill";
import { getDurationInHours } from "lib-a";
import { createZDT } from "lib-b";

const zdtA = createZDT(2022, 1, 1);
const zdtB = Temporal.ZonedDateTime.from({
  timeZone: "Asia/Tokyo",
  year: 2022,
  month: 1,
  day: 3,
  hour: 10,
});

console.log(getDurationInHours(zdtA, zdtB)); // TypeError: invalid result

[12wrigja]

Thanks - this is exactly what I think we need to fix. Can you create a branch somewhere I can clone to investigate this?

I was pondering whether we could fix the slots by storing all the slot information in an object attached to globalThis, referencing it using a named symbol that both versions of the package can find, something like

// in slots.ts
// replacing
const slots = new WeakMap();

// with
const globalTemporalSlotMapKey = Symbol.for('@@Temporal__slots__private_do_not_use');
const existingGlobalSlotMap = globalThis[globalTemporalSlotMapKey];
const slots = existingGlobalSlotMap || (globalThis[globalTemporalSlotMapKey] = new WeakMap());

[tars0x9752]

Sure! Here it is. #170

I added try-dual-pkg directory and there you will find the code to investigate dual package hazard. In order to fully reproduce the actual real world usage, each is written as a independent npm package (so no self-reference import here). There's a README.md in that directory and you can find more detailed instructions.

That globalThis approach looks good. It seemed to work fine when I tried it locally a little.

[12wrigja]

Thanks for drafting the PR! This looks very useful.

To set expectations: I'm currently pretty busy with other maintenance for this package and don't have time to dive into this problem, but I'm wondering if there might be a way we can make use of the extensive test262 test suite in some capacity to verify that "many" operations will "just work" if references to Temporal are mixed between the lib-esm and lib-cjs versions.

If the globalThis slots works, that would be good to pull out into it's own PR for us to deliberate / review.

[tars0x9752]

Ok! I'm not familiar with test262, so I'll look into it. And I'll try to work on the globalThis slots PR as well.

[stefee]

I wanted to add a note that we encountered this in the wild recently in a Next.js app which is served by a custom Node.js server. A Temporal.PlainDate for example, which is created by the Node.js server and then passed via asynchronous context to the Next.js server-side app, will cause some confusing errors when using compare methods for example. Next.js bundles the node_modules imports when running on the server, so the version of @js-temporal/polyfill in the Node.js context is different to the version of @js-temporal/polyfill in the Next.js context.

[12wrigja]

That is a rather interesting setup - is it possible for you to create a minimal reproduction of this problem we can use to verify our fixes will fix that problem too?

I'm wondering if you are instead running into an issue where some part of the polyfill implementation isn't being (de-)serialized completely by this context system, and that our fixes here might not be a solution.