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(marshal): cheap capData stringify and parse #1558

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions packages/marshal/src/stringify-capdata.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/** @template S @typedef {import('./types.js').CapData<S>} CapData */

const { Fail, quote: q } = assert;

/** @typedef {string} SimpleString */

/**
* A SimpleString is one where JSON.strigify-ing the string just
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* A SimpleString is one where JSON.strigify-ing the string just
* A SimpleString is one where JSON.stringify-ing the string just

* puts quotes around the contents, and it contains no close square
* brackets. Many slot encodings use slot strings that
* obey these rules, which is why they are useful.
*
* @param {unknown} str
* @returns {asserts str is SimpleString}
*/
export const assertSimpleString = str => {
assert(typeof str === 'string');
const stringified = JSON.stringify(str);
const expected = `"${str}"`;
stringified === expected ||
Fail`Expected to stringify to ${q(expected)}, not ${q(stringified)}}`;
assert(str.indexOf(']') === -1);
};
harden(assertSimpleString);

/**
* @param {CapData<SimpleString>} capData
Copy link
Member

Choose a reason for hiding this comment

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

since SimpleString = String this won't give a type error for plain string.

if you want to ensure that assertSimpleString was called first, the SimpleString type could have a tag so it's a superset of a regular string: The Opaque utility from type-fest makes this easy

* @returns {string}
*/
export const stringifyCapData = capData => {
const {
body,
slots: [...slots],
} = capData;
assert(typeof body === 'string');
assert(body.startsWith('#'));
Copy link
Contributor

Choose a reason for hiding this comment

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

If we do this, I hope it's in a way that expands the definition of "CapData" from { body: string, slots: Copyable[] } to e.g. { body: string, slots: Copyable[] } | { "#body": Copyable<encoding: 'smallcaps'>, slots: Copyable[] } and allows for direct production of the latter (Passable → CapData<format: 'smallcapsObject'> from e.g. marshaller.toCapData(passable), with subsequent JSON.stringify as warranted [and possibly after embedding in another structure]) rather than imposing a multi-step and non-embeddable process (Passable → CapData<bodyFormat: 'smallcapsString'> → JSONText).

const scBody = body.slice(1);
JSON.parse(scBody); // Just asserts that it parses as JSON
slots.forEach(assertSimpleString);
return `{"slots":${JSON.stringify(slots)},"#body":${scBody}}`;
};
harden(stringifyCapData);

const DSL = /^\{"slots":(\[[^\]]*\]),"#body":(.*)\}$/;

Choose a reason for hiding this comment

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

I would recommend to move to a less greedy search and string boundaries if possible
\A\{"slots":(\[[^\]]+?\]),"#body":(.+?)\}\Z

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had never seen \A or \Z before. Searching for it in JS RegExp documentation finally found it at https://docstore.mik.ua/orelly/webprog/jscript/ch10_01.htm in a section titled
"Perl RegExp Features Not Supported in JavaScript"

But good point about newlines. Where the (.*) appears, we should accept newlines along with everything else. IIUC, adding the s flag (aka "dotAll") on the end will give "." that meaning. Of course there are other ways to get the same effect by changing the pattern rather than adding a flag. What do you recommend?

Copy link
Contributor

Choose a reason for hiding this comment

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

Imposing an ordering constraint on the entries of a JSON object (which is defined to be unordered) would definitely merit explanation, as would the existence of this regular expression—I assume it's in anticipation of being able to look at slots without even parsing the body?

Copy link
Contributor

Choose a reason for hiding this comment

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

For checking the } at the end, s.endsWith('}') seems more straightforward. And playing around with RegExr suggest that \] doesn't match ].

Suggested change
const DSL = /^\{"slots":(\[[^\]]*\]),"#body":(.*)\}$/;
const DSL = /^{"slots":(\[[^\]]*]),"#body":/;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And playing around with RegExr suggest that \] doesn't match ].

image


export const parseCapData = str => {
assert(typeof str === 'string');
const matches = DSL.exec(str);
assert(matches && matches.length === 3);
const slots = JSON.parse(matches[1]);
slots.forEach(assertSimpleString);
const scBody = matches[2];
JSON.parse(scBody); // Just asserts that it parses as JSON
const body = `#${scBody}`;
return harden({ body, slots });
};
harden(parseCapData);
26 changes: 26 additions & 0 deletions packages/marshal/test/test-stringify-capdata.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { test } from './prepare-test-env-ava.js';

import {
assertSimpleString,
parseCapData,
stringifyCapData,
} from '../src/stringify-capdata.js';

test('test assertSimpleString', t => {
t.notThrows(() => assertSimpleString('x'));
t.throws(() => assertSimpleString('x"'), {
message: 'Expected to stringify to "\\"x\\"\\"", not "\\"x\\\\\\"\\""}',
});
t.throws(() => assertSimpleString('[x]'), {
message: 'Check failed',
});
});

const roundTrips = (t, capData, str) => {
t.is(stringifyCapData(capData), str);
t.deepEqual(parseCapData(str), capData);
};

test('test capData roundTrips', t => {
roundTrips(t, { body: '#[]', slots: ['a'] }, '{"slots":["a"],"#body":[]}');
});