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

feat(xsnap): deep stacks work with updated moddable error stacks #2579

Merged
merged 5 commits into from
Mar 5, 2021

Conversation

dckc
Copy link
Member

@dckc dckc commented Mar 4, 2021

I usually add a test... how important is that? What would / should it look like?

@dckc dckc requested review from kriskowal and erights March 4, 2021 20:08
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.

I usually add a test... how important is that? What would / should it look like?

For a test that runs after lockdown is called, how about something like:

test('something', t => {
  const foo = () => {
    const err = Error('msg');
    t.assert(!('stack' in err));
    const msg = getStack(err);
    t.is(typeof msg, 'string');
    t.assert(msg.length >= 1);
});

@dckc
Copy link
Member Author

dckc commented Mar 4, 2021

I get ReferenceError: get getStack: undefined variable. xsnap only uses the lockdown shim, not all of SES (recall we avoid babel). Does the lockdown shim provide getStack as a global?

@erights
Copy link
Member

erights commented Mar 4, 2021

I get ReferenceError: get getStack: undefined variable. xsnap only uses the lockdown shim, not all of SES (recall we avoid babel). Does the lockdown shim provide getStack as a global?

Ah sorry. getStackString.

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.

This is unspeakably exciting.

@@ -5,7 +5,7 @@ export default [
{
input: 'lib/bootstrap.js',
output: {
file: `dist/bootstrap.umd.js`,
file: `dist/bundle-ses-boot.umd.js`,
Copy link
Member

Choose a reason for hiding this comment

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

I worry that this obscures the provenance of the file, but I assume this is motivated by IDE auto-complete to open a file, which is worthy. Maybe we could stutter dist/dist-bootstrap.js? Whatever we do, it should be a rule we follow elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

The rule this change is designed to follow is:

# bundles
bundle-*.js

https://github.com/Agoric/agoric-sdk/blob/master/.gitignore#L56-L57

I'm not sure what you mean by "this" in "I worry that this obscures..." A change from bootstrap to bundle-sys-boot obscures?

Maybe I should get rid of this bundle in favor of a t.before function. It was only used in tests, originally. It's now used in ava-xs, but ava-xs also uses bundleSource on test scripts, so it might as well bundle this at runtime too.

Copy link
Member

Choose a reason for hiding this comment

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

I’d be down for, as a rule, naming bundle-x.js for every input x.js. That would imply, for example, dist/bundle-ses.umd.js and dist/bundle-lockdown.umd.js elsewhere, and bundle-bootstrap.umd.js here.

Copy link
Member

@kriskowal kriskowal Mar 5, 2021

Choose a reason for hiding this comment

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

Or rather, it would be. I think we depend too heavily on the exact names in ses/dist. In some cases the dist files are part of the public API.

Copy link
Member Author

Choose a reason for hiding this comment

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

it seems like the name of this file is independent of deep stacks... but I guess I raised the issue when I renamed it in this PR.

Currently, ava-xs is the only consumer. And having it in the xsnap package is increasingly awkward. But I expect xsnap to move to SES-shim (or endo) in due course, so I'm inclined to not bother with this just now. I suppose I owe an issue about it or something...

@dckc dckc enabled auto-merge (squash) March 5, 2021 23:07
@dckc dckc merged commit 6a8fc76 into master Mar 5, 2021
@dckc dckc deleted the xs-update-deepstacks branch March 5, 2021 23:21
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.

3 participants