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

Add Fragment Node #23

Merged
merged 13 commits into from
Feb 9, 2019
Merged

Add Fragment Node #23

merged 13 commits into from
Feb 9, 2019

Conversation

stephencelis
Copy link
Member

This tweet points out a problem with the current API:

https://twitter.com/hoagy_ytfc/status/1079550683253219328

Without being able to represent a list of nodes as a node itself, we can't currently use Vapor's protocol-oriented ResponseEncodable API to return a doctype alongside more root nodes:

[
  doctype,
  html([
    // …
    ])
]

One solution is to add a fragment node, which represents a document or fragment of HTML. The DOM spec has included a DocumentFragment node type for a very long time and React recently added its own Fragment API, so the precedent is there. We've also struggled with our View construct in the past as to whether we should be composing (A) -> Node or (A) -> [Node], so this could help solve that problem.

The change in the PR results in:

Node.fragment([
  doctype,
  html([
    // …
    ])
  ])

A few unanswered questions:

  1. Node.fragment is pretty verbose. Even when there's inference, .fragment([…]) might not read well. Do we wanna think of alternative names? Do we want to add a document function as an alias?

    document([
      doctype,
      html([
        // …
        ])
      ])
  2. Do we remove the render([Node]) overload or keep it around as convenience?

  3. Do we add ExpressibleByArrayLiteral or are we worried about Swift diagnostics?

@stephencelis
Copy link
Member Author

One other consideration: if we start composing HTML from a bunch of fragments will it affect performance?

}

public prefix func ... (nodes: [Node]) -> Node {
return .fragment(nodes.filter { !$0.isEmpty })
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I understand why this filter is needed. What's up with it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Was just trying to get rid of the nesting and slipped it in here. Probably not necessary or probably better as an explicit traversal?

case .element:
return false
case let .fragment(children):
return children.isEmpty || children.allSatisfy { $0.isEmpty }
Copy link
Member

Choose a reason for hiding this comment

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

you can drop the children.isEmpty since allSatisfy returns true for empty collections.

```

This makes the “Swiftification” of an HTML document looks very similar to the original document.

## FAQ

<!--
Copy link
Member

Choose a reason for hiding this comment

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

ha oops! wonder how that slipped in

Sources/Html/Node.swift Outdated Show resolved Hide resolved
mbrandonw and others added 3 commits February 8, 2019 18:51
@stephencelis stephencelis merged commit 4e931b2 into master Feb 9, 2019
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.

2 participants