Skip to content

Commit

Permalink
handle errors in args
Browse files Browse the repository at this point in the history
  • Loading branch information
@patricklx
patricklx committed Nov 2, 2023
1 parent 68d371b commit 626b0e7
Show file tree
Hide file tree
Showing 6 changed files with 94 additions and 27 deletions.
63 changes: 41 additions & 22 deletions packages/@glimmer-workspace/integration-tests/test/debug-render-tree-test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import {
JitRenderDelegate,
RenderTest,
suite,
test,
test, tracked,
} from '..';

interface CapturedBounds {
Expand Down Expand Up @@ -90,7 +90,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first' }, errors: {} },
instance: null,
template: '(unknown template module)',
bounds: this.nodeBounds(this.delegate.getInitialElement().firstChild),
Expand All @@ -104,7 +104,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first' }, errors: {} },
instance: null,
template: '(unknown template module)',
bounds: this.nodeBounds(this.delegate.getInitialElement().firstChild),
Expand All @@ -113,7 +113,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld',
args: { positional: [], named: { arg: 'second' } },
args: { positional: [], named: { arg: 'second' }, errors: {} },
instance: null,
template: '(unknown template module)',
bounds: this.nodeBounds(this.delegate.getInitialElement().lastChild),
Expand All @@ -127,7 +127,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first' }, errors: {} },
instance: null,
template: '(unknown template module)',
bounds: this.nodeBounds(this.delegate.getInitialElement().firstChild),
Expand All @@ -138,33 +138,52 @@ class DebugRenderTreeTest extends RenderTest {

@test 'emberish curly components'() {
this.registerComponent('Curly', 'HelloWorld', 'Hello World');
let error: Error|null = null;
class State {
@tracked doFail = false;
get getterWithError() {
if (!this.doFail) return;
error = new Error('error');
throw error;
}
}
const obj = new State();

this.render(
`<HelloWorld @arg="first"/>{{#if this.showSecond}}<HelloWorld @arg="second"/>{{/if}}`,
`<HelloWorld @arg="first" @arg2={{this.obj.getterWithError}}/>{{#if this.showSecond}}<HelloWorld @arg="second"/>{{/if}}`,
{
showSecond: false,
obj,
}
);

obj.doFail = true;

this.delegate.getCapturedRenderTree();

this.assert.ok(error !== null, 'expecting an Error');

this.assertRenderTree([
{
type: 'component',
name: 'HelloWorld',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first', arg2: error }, errors: { arg2: error! } },

This comment has been minimized.

Sign in to view

Copy link
@chancancode

chancancode Nov 11, 2023

Contributor

This seems wrong: if the arts interface is { positional, named }, shouldn't this new object also need to have { errors: { positional, named } } in it?

Also, it may be more practical to put the error object in place of the arg that it would have replaced, and flag it, rather than having an auxiliary data structure as a side channel. I suspect that would make the inspector code a bit easier to write, since you can just render the arguments the same way but replace the icon and/or make the name red or something.

export interface ArgumentError {
  message: string;
  cause: unknown;
}

const ARGUMENT_ERROR = Symbol("ARGUMENT_ERROR");

export function isArgumentError(arg: unknown): arg is ArgumentError {
  return arg !== null && typeof arg === "object" && arg[ARGUMENT_ERROR];
}

class ArgumentErrorImpl {
  [ARGUMENT_ERROR] = true;

  constructor(readonly cause: unknown) {
    super("an error occurred while capturing this argument");
  }
}

This comment has been minimized.

Sign in to view

Copy link
@patricklx

patricklx Nov 11, 2023 •
edited
Loading

Author Contributor

I was about to implement this now. But i the end it makes no difference in the inspector.
The inspector serializes it here:
https://github.com/emberjs/ember-inspector/blob/ba75238bc4f1e8cdea83d41ebc4761f2e189f66f/ember_debug/libs/render-tree.js#L517.
So i would need to change what information is sent to the inspector there.
I would prefer not to need to require an internal function to check if something is a caught error or not.
But yes, the interface should be the same then. I will fix that

This comment has been minimized.

Sign in to view

Copy link
@chancancode

chancancode Nov 12, 2023

Contributor

That part of the code is just essentially what is consuming/walking the tree, it is true that you would make changes there, but I wouldn't call it making no difference. As it is, you would have to pass along the side-channel information to all the serialize* and do some kind of zip operation to pair up the two data structures while iterating over them.

I think the current interface we merged here is pretty awkward, and essentially duplicates a subset of the args twice, and you would have to invent some kind of way to deal with the positional arguments in that side-channel as well – is it going to be errors: { positional: [null, someError, null, anotherError], named: { ... } }? what if the error being throw is null (or false or whatever you picked)? Remember "errors" you caught in JavaScript can be any value, not just subclasses of Error.

This comment has been minimized.

Sign in to view

Copy link
@patricklx

patricklx Nov 12, 2023

Author Contributor

True... Then how about wrapping the thrown error in ArgumentError class and use a property like isErrorCaughtByGlimmerVM = true instead of a symbol? Then we do not need to require internal things. Though it would be possible for someone else to set that property on their own throw errors. But i think that would be ok?

This comment has been minimized.

Sign in to view

Copy link
@chancancode

chancancode Nov 12, 2023

Contributor

We could, but we could also just tack _isArgumentCaptureError to wherever _debugRenderTree already comes from. Looks like we already have to do something like this: https://github.com/emberjs/ember-inspector/blob/ba75238bc4f1e8cdea83d41ebc4761f2e189f66f/ember_debug/libs/render-tree.js#L19

So if it's just the difficulty of exposing that one function, probably not a bid deal to stick it in there too

This comment has been minimized.

Sign in to view

Copy link
@patricklx

patricklx Nov 12, 2023

Author Contributor

On the errors structure. I was thinking to change it to just be true or false values. It is just to indicate if a value is detected as an error

This comment has been minimized.

Sign in to view

Copy link
@patricklx

patricklx Nov 12, 2023

Author Contributor

Ah, if the function is put in the debug render tree I'm okay with it
instance: (instance: EmberishCurlyComponent) => (instance as any).arg === 'first',
template: '(unknown template module)',
bounds: this.nodeBounds(this.delegate.getInitialElement().firstChild),
children: [],
},
]);

obj.doFail = false;

this.rerender({ showSecond: true });

this.assertRenderTree([
{
type: 'component',
name: 'HelloWorld',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first', arg2: undefined }, errors: {} },
instance: (instance: EmberishCurlyComponent) => (instance as any).arg === 'first',
template: '(unknown template module)',
bounds: this.nodeBounds(this.element.firstChild),
Expand All @@ -173,7 +192,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld',
args: { positional: [], named: { arg: 'second' } },
args: { positional: [], named: { arg: 'second' }, errors: {} },
instance: (instance: EmberishCurlyComponent) => (instance as any).arg === 'second',
template: '(unknown template module)',
bounds: this.nodeBounds(this.element.lastChild),
Expand All @@ -187,7 +206,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first', arg2: undefined }, errors: {} },
instance: (instance: EmberishCurlyComponent) => (instance as any).arg === 'first',
template: '(unknown template module)',
bounds: this.nodeBounds(this.element.firstChild),
Expand All @@ -210,7 +229,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first' }, errors: {} },
instance: (instance: GlimmerishComponent) => instance.args['arg'] === 'first',
template: '(unknown template module)',
bounds: this.nodeBounds(this.delegate.getInitialElement().firstChild),
Expand All @@ -224,7 +243,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first' }, errors: {} },
instance: (instance: GlimmerishComponent) => instance.args['arg'] === 'first',
template: '(unknown template module)',
bounds: this.nodeBounds(this.element.firstChild),
Expand All @@ -233,7 +252,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld',
args: { positional: [], named: { arg: 'second' } },
args: { positional: [], named: { arg: 'second' }, errors: {} },
instance: (instance: GlimmerishComponent) => instance.args['arg'] === 'second',
template: '(unknown template module)',
bounds: this.nodeBounds(this.element.lastChild),
Expand All @@ -247,7 +266,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first' }, errors: {} },
instance: (instance: GlimmerishComponent) => instance.args['arg'] === 'first',
template: '(unknown template module)',
bounds: this.nodeBounds(this.element.firstChild),
Expand Down Expand Up @@ -307,7 +326,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld2',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first' }, errors: {} },
instance: null,
template: '(unknown template module)',
bounds: this.nodeBounds(this.delegate.getInitialElement().firstChild),
Expand All @@ -321,7 +340,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld2',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first' }, errors: {} },
instance: null,
template: '(unknown template module)',
bounds: this.nodeBounds(this.element.firstChild),
Expand All @@ -330,15 +349,15 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'route-template',
name: 'foo',
args: { positional: [], named: { arg: 'second' } },
args: { positional: [], named: { arg: 'second' }, errors: {} },
instance: instance1,
template: null,
bounds: this.nodeBounds(this.element.lastChild),
children: [
{
type: 'engine',
name: 'bar',
args: { positional: [], named: {} },
args: { positional: [], named: {}, errors: {} },
instance: instance2,
template: null,
bounds: this.nodeBounds(this.element.lastChild),
Expand All @@ -354,7 +373,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld2',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first' }, errors: {} },
instance: null,
template: '(unknown template module)',
bounds: this.nodeBounds(this.element.firstChild),
Expand Down Expand Up @@ -387,7 +406,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld2',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first' }, errors: {} },
instance: null,
template: '(unknown template module)',
bounds: this.nodeBounds(this.delegate.getInitialElement().firstChild),
Expand All @@ -401,7 +420,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld2',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first' }, errors: {} },
instance: null,
template: '(unknown template module)',
bounds: this.nodeBounds(this.element.firstChild),
Expand All @@ -415,7 +434,7 @@ class DebugRenderTreeTest extends RenderTest {
{
type: 'component',
name: 'HelloWorld2',
args: { positional: [], named: { arg: 'first' } },
args: { positional: [], named: { arg: 'first' }, errors: {} },
instance: null,
template: '(unknown template module)',
bounds: this.nodeBounds(this.element.firstChild),
Expand Down
6 changes: 6 additions & 0 deletions packages/@glimmer/interfaces/lib/runtime/arguments.d.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,9 @@ export interface Arguments {
positional: readonly unknown[];
named: Record<string, unknown>;
}

export interface ArgumentsDebug {
positional: readonly unknown[];
named: Record<string, unknown>;
errors: Record<string, Error>;
}
4 changes: 2 additions & 2 deletions packages/@glimmer/interfaces/lib/runtime/debug-render-tree.d.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { SimpleElement, SimpleNode } from '@simple-dom/interface';

import type { Bounds } from '../dom/bounds';
import type { Arguments, CapturedArguments } from './arguments';
import type { ArgumentsDebug, CapturedArguments } from './arguments';

export type RenderNodeType = 'outlet' | 'engine' | 'route-template' | 'component';

Expand All @@ -17,7 +17,7 @@ export interface CapturedRenderNode {
id: string;
type: RenderNodeType;
name: string;
args: Arguments;
args: ArgumentsDebug;
instance: unknown;
template: string | null;
bounds: null | {
Expand Down
4 changes: 2 additions & 2 deletions packages/@glimmer/runtime/lib/debug-render-tree.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import type {
} from "@glimmer/interfaces";
import { assign, expect, Stack } from '@glimmer/util';

import { reifyArgs } from './vm/arguments';
import { reifyArgsDebug } from './vm/arguments';

interface InternalRenderNode<T extends object> extends RenderNode {
bounds: Nullable<Bounds>;
Expand Down Expand Up @@ -181,7 +181,7 @@ export default class DebugRenderTreeImpl<TBucket extends object>
let template = this.captureTemplate(node);
let bounds = this.captureBounds(node);
let children = this.captureRefs(refs);
return { id, type, name, args: reifyArgs(args), instance, template, bounds, children };
return { id, type, name, args: reifyArgsDebug(args), instance, template, bounds, children };
}

private captureTemplate({ template }: InternalRenderNode<TBucket>): Nullable<string> {
Expand Down
42 changes: 42 additions & 0 deletions packages/@glimmer/runtime/lib/vm/arguments.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -500,6 +500,48 @@ export function reifyArgs(args: CapturedArguments) {
};
}

export function reifyNamedDebug(named: CapturedNamedArguments) {
let reified = dict();
let errors: Record<string, Error> = dict();

for (const [key, value] of Object.entries(named)) {
try {
reified[key] = valueForRef(value);
} catch (e) {
reified[key] = e;
errors[key] = e as Error;
}
}

return { reified, errors };
}

export function reifyPositionalDebug(positional: CapturedPositionalArguments) {
let errors: Error[] = [];
let reified = positional.map((p) => {
try {
return valueForRef(p);
} catch(e) {
errors.push(e as Error);
return e;
}
});
return {
reified,
errors
}
}

export function reifyArgsDebug(args: CapturedArguments) {
let named = reifyNamedDebug(args.named);
let positional = reifyPositionalDebug(args.positional);
return {
named: named.reified,
positional: positional.reified,
errors: {...named.errors, ...positional.errors} as unknown as Record<string, Error >

This comment has been minimized.

Sign in to view

Copy link
@chancancode

chancancode Nov 11, 2023

Contributor

This hides an actual error
};
}

export const EMPTY_NAMED = Object.freeze(Object.create(null)) as CapturedNamedArguments;
export const EMPTY_POSITIONAL = EMPTY_REFERENCES as CapturedPositionalArguments;
export const EMPTY_ARGS = createCapturedArgs(EMPTY_NAMED, EMPTY_POSITIONAL);
2 changes: 1 addition & 1 deletion packages/@glimmer/vm-babel-plugins/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export default function generateVmPlugins(
return [
[
__loadPlugins
?
?
require('babel-plugin-debug-macros')
: require.resolve('babel-plugin-debug-macros'),
{
Expand Down

0 comments on commit 626b0e7

Please sign in to comment.