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

fix(xsnap): format objects nicely in console using SES assert.quote #3856

Merged
merged 6 commits into from
Sep 21, 2021

Conversation

dckc
Copy link
Member

@dckc dckc commented Sep 20, 2021

fixes #3844

A couple things are a little awkward but seem cost-effective:

  • This uses setQuote() to break the cycle where SES requires console and console is implemented using assert.quote from SES.
  • for testing I added a global to capture args from console methods via print().

@dckc dckc requested review from kriskowal and erights September 20, 2021 21:03
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

packages/xsnap/lib/console-shim.js Outdated Show resolved Hide resolved
packages/xsnap/lib/console-shim.js Outdated Show resolved Hide resolved
@dckc dckc enabled auto-merge (squash) September 20, 2021 21:24
print(` ${i}:`, a.toString ? a.toString() : '<no .toString>', typeof a);
});
}
// eslint-disable-next-line
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you disabling here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eslint doesn't know that xsnap has globalThis.print.

Copy link
Member Author

@dckc dckc Sep 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no-undef was not enough somehow.

note the eslint-disable-next-line was there before this change. twice three times, in fact.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment explaining this, so it won't look so mysterious to the next reader.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/* global print */ also works, and TSC interprets it as print : any.

@dckc dckc disabled auto-merge September 20, 2021 21:25
@dckc dckc requested a review from erights September 20, 2021 21:31
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

packages/xsnap/lib/console-shim.js Outdated Show resolved Hide resolved
packages/xsnap/lib/console-shim.js Show resolved Hide resolved
packages/xsnap/lib/console-shim.js Outdated Show resolved Hide resolved
@dckc dckc enabled auto-merge (squash) September 20, 2021 21:47
print(` ${i}:`, a.toString ? a.toString() : '<no .toString>', typeof a);
});
}
// eslint-disable-next-line
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment explaining this, so it won't look so mysterious to the next reader.

@dckc dckc disabled auto-merge September 20, 2021 21:49
@dckc dckc force-pushed the 3844-xsnap-console-fmt branch 2 times, most recently from 5ad8e36 to 1da421f Compare September 20, 2021 22:36
@dckc dckc enabled auto-merge (squash) September 20, 2021 22:38
@dckc dckc force-pushed the 3844-xsnap-console-fmt branch from 1da421f to 291f914 Compare September 21, 2021 15:16
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

Successfully merging this pull request may close these issues.

[object Object] obscure form in xsnap console
3 participants