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

Introduces slots #356

Merged
merged 2 commits into from
Apr 17, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .vscode/launch.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
"runtimeArgs": ["-r", "ts-eager/register"],
"console": "integratedTerminal",
"program": "${workspaceFolder}/spec/marktest/index.ts",
"cwd": "${workspaceFolder}/spec/marktest",
"cwd": "${workspaceFolder}",
"args": ["${file}:${lineNumber}"]
}
]
Expand Down
9 changes: 6 additions & 3 deletions index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import transforms from './src/transforms';
import { parseTags, isPromise } from './src/utils';
import validator from './src/validator';

import type { Node } from './src/types';
import type { Node, ParserArgs } from './src/types';
import type Token from 'markdown-it/lib/token';
import type { Config, RenderableTreeNode, ValidateError } from './src/types';

Expand All @@ -39,9 +39,12 @@ function mergeConfig(config: Config = {}): Config {
};
}

export function parse(content: string | Token[], file?: string): Node {
export function parse(
content: string | Token[],
args?: string | ParserArgs
): Node {
if (typeof content === 'string') content = tokenizer.tokenize(content);
return parser(content, file);
return parser(content, args);
}

export function resolve<C extends Config = Config>(
Expand Down
8 changes: 4 additions & 4 deletions spec/marktest/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,9 @@ const tokenizer = new markdoc.Tokenizer({
allowComments: true,
});

function parse(content: string, file?: string) {
function parse(content: string, slots?: boolean, file?: string) {
const tokens = tokenizer.tokenize(content);
return markdoc.parse(tokens, file);
return markdoc.parse(tokens, { file, slots });
}

function stripLines(object) {
Expand All @@ -36,7 +36,7 @@ function stripLines(object) {
function render(code, config, dynamic) {
const partials = {};
for (const [file, content] of Object.entries(config.partials ?? {}))
partials[file] = parse(content as string, file);
partials[file] = parse(content as string, false, file);

const { react, reactStatic } = markdoc.renderers;
const transformed = markdoc.transform(code, { ...config, partials });
Expand Down Expand Up @@ -105,7 +105,7 @@ function formatValidation(filename, test, validation) {

let exitCode = 0;
for (const test of tests) {
const code = parse(test.code || '');
const code = parse(test.code || '', test.slots);

const { start, end } = test.$$lines;
if (line && (Number(line) - 1 < start || Number(line) - 1 > end)) continue;
Expand Down
194 changes: 194 additions & 0 deletions spec/marktest/tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1592,3 +1592,197 @@
code: |
{% foo bar="this is a test of \"quoted\" strings" /%}
expected: <article><foo bar="this is a test of &quot;quoted&quot; strings"></foo></article>

- name: Basic slot
config:
tags:
foo:
render: foo
slots:
bar: {}
code: |
{% foo %}
{% slot "bar" %}
This is a test
{% /slot %}
{% /foo %}
slots: true
expected:
- tag: foo
attributes:
bar:
- tag: p
children:
- This is a test

- name: Tag with multiple slots and additional content
config:
tags:
foo:
render: foo
attributes:
qux:
type: String
slots:
bar: {}
baz: {}
code: |
{% foo qux="test" %}
{% slot "bar" %}
This is a test
{% /slot %}
{% slot "baz" %}
This is **another** test
{% /slot %}
This is additional content
{% /foo %}
slots: true
expected:
- tag: foo
attributes:
qux: test
bar:
- tag: p
children:
- This is a test
baz:
- tag: p
children:
- 'This is '
- tag: strong
children: [another]
- ' test'
children:
- tag: p
children:
- This is additional content

- name: User slot tag when slots are disabled
config:
tags:
slot:
render: foo
code: |
{% slot %}
bar
{% /slot %}
expected:
- tag: foo
children:
- tag: p
children: [bar]

- name: Handling slots that are missing a name
config:
tags:
foo:
render: foo
attributes:
bar:
type: Node
code: |
{% foo %}
{% slot %}
This is a test
{% /slot %}
{% /foo %}
slots: true
expectedError: "Missing required attribute: 'primary'"
expected:
- tag: foo
children: [null]

- name: Handling slots with invalid name
config:
tags:
foo:
render: foo
attributes:
bar:
type: Node
code: |
{% foo %}
{% slot 1 %}
This is a test
{% /slot %}
{% /foo %}
slots: true
expectedError: "Attribute 'primary' must be type of 'String'"
expected:
- tag: foo
children: [null]

- name: Validating required slot
config:
tags:
foo:
render: foo
slots:
bar:
required: true
code: |
{% foo %}
{% /foo %}
slots: true
expectedError: "Missing required slot: 'bar'"
expected:
- tag: foo

- name: Handling invalid slot
config:
tags:
foo:
render: foo
code: |
{% foo %}
{% slot "bar" %}
This is a test
{% /slot %}
{% /foo %}
slots: true
expectedError: "Invalid slot: 'bar'"
expected:
- tag: foo

- name: Handling overlapping slot and attribute
config:
tags:
foo:
render: foo
attributes:
bar:
type: String
slots:
bar: {}
code: |
{% foo bar="test" %}
{% /foo %}
{% foo bar="test" %}
{% slot "bar" %}
test
{% /slot %}
{% /foo %}
{% foo %}
{% slot "bar" %}
test
{% /slot %}
{% /foo %}
slots: true
expected:
- tag: foo
attributes:
bar: 'test'
- tag: foo
attributes:
bar:
- tag: p
children: [test]
- tag: foo
attributes:
bar:
- tag: p
children: [test]
7 changes: 7 additions & 0 deletions src/ast/node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export default class Node implements AstType {
readonly $$mdtype = 'Node';

attributes: Record<string, any>;
slots: Record<string, Node>;
children: Node[];
errors: ValidationError[] = [];
lines: number[] = [];
Expand All @@ -38,9 +39,15 @@ export default class Node implements AstType {
this.type = type;
this.tag = tag;
this.annotations = [];
this.slots = {};
}

*walk(): Generator<Node, void, unknown> {
for (const slot of Object.values(this.slots)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be faster to make this.slots default to undefined and avoid the Object.values(this.slots) in the default case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess alternatively you could simplify this function by doing:

    for (const child of this.children.concat(Object.values(this.slots))) {
      ...

or something similar

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'm changing it to: for (const child of [...Object.values(this.slots), ...this.children]) {. I don't think it materially impacts performance either way.

yield slot;
yield* slot.walk();
}

for (const child of this.children) {
yield child;
yield* child.walk();
Expand Down
42 changes: 40 additions & 2 deletions src/parser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,50 @@ describe('Markdown parser', function () {
const fence = '```';
const tokenizer = new Tokenizer({ allowComments: true });

function convert(example) {
function convert(example, options?) {
const content = example.replace(/\n\s+/gm, '\n').trim();
const tokens = tokenizer.tokenize(content);
return parser(tokens);
return parser(tokens, options);
}

describe('handling options', function () {
it('no args', function () {
const example = convert(`# This is a test`);
expect(example.children[0]).toDeepEqual({
...any(),
type: 'heading',
location: {
...any(),
file: undefined,
},
});
});

it('filename as string', function () {
const example = convert(`# This is a test`, 'foo.md');
expect(example.children[0]).toDeepEqual({
...any(),
type: 'heading',
location: {
...any(),
file: 'foo.md',
},
});
});

it('filename as property', function () {
const example = convert(`# This is a test`, { file: 'foo.md' });
expect(example.children[0]).toDeepEqual({
...any(),
type: 'heading',
location: {
...any(),
file: 'foo.md',
},
});
});
});

describe('handling frontmatter', function () {
it('simple frontmatter', function () {
const example = convert(`
Expand Down
21 changes: 16 additions & 5 deletions src/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import Node from './ast/node';
import transforms from './transforms/index';
import { OPEN } from './utils';

import type { AttributeValue } from './types';
import type { AttributeValue, ParserArgs } from './types';

import type Token from 'markdown-it/lib/token';

Expand Down Expand Up @@ -88,6 +88,7 @@ function handleToken(
token: Token,
nodes: Node[],
file?: string,
handleSlots?: boolean,
inlineParent?: Node
) {
if (token.type === 'frontmatter') {
Expand Down Expand Up @@ -155,7 +156,14 @@ function handleToken(
if (attributes && ['tag', 'fence', 'image'].includes(typeName))
annotate(node, attributes);

parent.push(node);
if (
handleSlots &&
tag === 'slot' &&
typeof node.attributes.primary === 'string'
)
parent.slots[node.attributes.primary] = node;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make children a reserved slot key? Or is there no reason to avoid this?

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 don't think this is necessary. In the event that someone uses children as the name of the slot, the value will appear as expected in the render tree but will be replaced by the actual children in the React props.

This is already the behavior we have today with attributes and we don't enforce any restrictions to prevent that. React itself does pretty much the same thing if use a "children" attribute in JSX as well as passing in actual children.

else parent.push(node);

if (token.nesting > 0) nodes.push(node);

if (!Array.isArray(token.children)) return;
Expand All @@ -167,17 +175,20 @@ function handleToken(
const isLeafNode = typeName === 'image';
if (!isLeafNode) {
for (const child of token.children)
handleToken(child, nodes, file, inlineParent);
handleToken(child, nodes, file, handleSlots, inlineParent);
}

nodes.pop();
}

export default function parser(tokens: Token[], file?: string) {
export default function parser(tokens: Token[], args?: string | ParserArgs) {
mfix-stripe marked this conversation as resolved.
Show resolved Hide resolved
const doc = new Node('document');
const nodes = [doc];

for (const token of tokens) handleToken(token, nodes, file);
if (typeof args === 'string') args = { file: args };

for (const token of tokens)
handleToken(token, nodes, args?.file, args?.slots);
mfix-stripe marked this conversation as resolved.
Show resolved Hide resolved

if (nodes.length > 1)
for (const node of nodes.slice(1))
Expand Down