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

Expose _repr_ in non-debug builds #227

Open
lionel-rowe opened this issue Mar 1, 2023 · 7 comments
Open

Expose _repr_ in non-debug builds #227

lionel-rowe opened this issue Mar 1, 2023 · 7 comments

Comments

@lionel-rowe
Copy link

Currently, logging Temporal objects imported via the npm module in Node or via esm.sh in the browser, Deno, etc. yields pretty unhelpful results:

console.log(new Temporal.Instant(0n))
// Temporal.Instant {}

Exposing _repr_ on non-debug builds would massively improve this experience:

console.log(new Temporal.Instant(0n))
// Temporal.Instant { _repr_: "Temporal.Instant <1970-01-01T00:00:00Z>" }

Cost to bundle size is ~2k uncompressed. Would this be acceptable? Or is there some other way the logging/debugging output could be improved?

@justingrant
Copy link
Contributor

Once Temporal ships natively in browsers (I expect this to happen later this year or early 2024) then presumably browsers' debug consoles will natively expose more useful debug console output, much like they do for Date, Set, etc.

For polyfills, the goal is to mirror the spec exactly, so exposing _repr_ outside debug builds doesn't sound like something that we'd want to do. So it's less about bundle size and more that we don't want to have production users start expecting a property that won't be in native Temporal objects.

Or is there some other way the logging/debugging output could be improved?

I'm admittedly unfamiliar with whether there's a better way to do this. If you have ideas, especially those that won't change the observable properties and methods of objects, we're open to it.

@lionel-rowe
Copy link
Author

lionel-rowe commented Mar 1, 2023

Hmm, makes sense. For now I'm using this wrapper in Deno — similar would work for Node but with Symbol.for('nodejs.util.inspect.custom') instead:

import { Temporal } from 'https://esm.sh/@js-temporal/polyfill@0.4.3'

for (const k of [
	'Instant',
	'Calendar',
	'PlainDate',
	'PlainDateTime',
	'Duration',
	'PlainMonthDay',
	'PlainTime',
	'TimeZone',
	'PlainYearMonth',
	'ZonedDateTime',
] as const) {
	const prototype = (Temporal[k]).prototype as any

	prototype[Symbol.for('Deno.customInspect')] = function() {
		return `\x1b[36m${this[Symbol.toStringTag]} <${this.toString()}>\x1b[0m`
	}
}

export { Temporal }

I can't think of anything similar that'd work in browser consoles, though, as they'll only show non-computed properties, and it's not possible to overwrite a class's constructor... unless unique symbols don't count as "observable" for this purpose? In which case it'd be trivial for the library to expose a unique Symbol('_repr_') that would show up in logging on any platform:

const reprSymbol = Symbol('_repr_')
class X { [reprSymbol] = 'xyz' }
console.log(new X())
// logs something similar to "X { [Symbol(_repr_)]: 'xyz' }" in all of Chrome, FireFox, Deno, Node, etc

@ptomato
Copy link
Contributor

ptomato commented Mar 1, 2023

Your PR #226 seemed fine for the purpose of getting custom logging working in Deno? Or is the problem that it needs to work in non-debug builds?

In the proposal's browser playground we define window.devtoolsFormatters (link) for Chrome, which has the advantage of not adding any properties to Temporal objects, but is otherwise pretty inconvenient because in Chrome devtools, custom formatters aren't switched on by default. You have to enable them specially.

Symbol('_repr_') would unfortunately be observable; you can observe it with Object.getOwnPropertySymbols() or Object.getOwnPropertyDescriptors(). What we're trying to avoid is some facility that looks like an API, that users of the polyfill might be tempted to use in their code, which would break when they stopped using the polyfill.

@12wrigja
Copy link
Contributor

12wrigja commented Mar 1, 2023

To clarify our current build setup: nothing we publish to NPM (sans a maintainer error when publishing) would include debug code as that is optimized out before publishing.

If you are debugging your code using local builds, you could instead locally link to our sources and create a local debug build of your project by cloning the repo and using npm link to link together your local builds with your project:

git clone https://github.com/js-temporal/temporal-polyfill.git
cd temporal-polyfill && git checkout v<release tag here>
cd <to your project>
npm link <relative path to the temporal-polyfill directory>
// then run your builds like normal and see the debug symbols present.

More steps to follow, but this procedure works for any NPM package with nothing specific to this project.

@lionel-rowe
Copy link
Author

Your PR #226 seemed fine for the purpose of getting custom logging working in Deno? Or is the problem that it needs to work in non-debug builds?

Yeah, I figured it won't work for my use case as I'm using Node when contributing to the library, whereas when I'm consuming it in Deno I import from an esm.sh URL that builds from the NPM version. It's true that I can compile my own version with a different NODE_ENV setting, but at that point the wrapper + monkey-patch solution is easier anyway, as it doesn't require recompiling every time the library is updated.

@lionel-rowe
Copy link
Author

If you have ideas, especially those that won't change the observable properties and methods of objects, we're open to it.

Now I think about it, wouldn't a private field fulfil this requirement? There's no way to access them from outside the class itself, including in derived classes. Problem is Babel compiles them to a WeakMap-based implementation that won't show up in dev tools, even when env.TRANSPILE isn't set. Not sure why that's the case — maybe babelConfig: undefined still defaults to targeting ES5 (?!)

Private fields also won't show up for Node/Deno logging in the terminal (but will show up when inspecting Node/Deno processes in dev tools).

@ptomato
Copy link
Contributor

ptomato commented Mar 8, 2023

The private field idea sounds nice - it'd provide at least some sort of insight into the object in non-debug builds, at least if you were using devtools.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants