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(parser): add support for custom selfClosing tags #21

Closed
wants to merge 15 commits into from

Conversation

michael-ciniawsky
Copy link
Member

@michael-ciniawsky michael-ciniawsky commented Dec 19, 2017

Notable Changes

  • adds support for custom selfClosing tags
const parser = require('posthtml-parser')

const html = '<import src="path/to/file.html">'

const tree = parser(options)(html)

[
  {
    tag: 'import'
    attrs: {
      src: 'path/to/file.html'
    }
  }
]

BREAKING CHANGES

  • drops support for node < v4.0.0
  • parser is curried
parser(options)(html)

Issues

PR's

@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented Dec 19, 2017

options.selfClosing might be a better name for this, waiting for feedback here

I could also add support for node < v4.0.0 back, but would really love not to (node < v4.0.0 is EOL more than a year now)

lib/parser.js Outdated

const result = []

const selfClosing = SELF_CLOSING.concat(options.tags || [])
Copy link
Member Author

Choose a reason for hiding this comment

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

Relevant

Copy link
Member

@Scrum Scrum Dec 22, 2017

Choose a reason for hiding this comment

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

no, you need to add a parameter selfClosing in tree, and check if the tag has a closing slash and does not have content

<customTag />
tag: 'customTag'

Copy link
Member Author

@michael-ciniawsky michael-ciniawsky Dec 22, 2017

Choose a reason for hiding this comment

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

I'm not sure I fully understand, are you suggesting to add a selfClosing prop to the node instead ?

{
  tag: 'import'
  selfClosing: true // <=
  attrs: { ... }
}

?

The SELF_CLOSING list of tags might be unneeded here, I definitely need to revisit this once more, maybe I mixed things up here 😛 and we only need to check the node shape
to not have any content, like you suggested

lib/parser.js Outdated
'source',
'track',
'wbr',
// Custom (PostHTML)
Copy link
Member Author

Choose a reason for hiding this comment

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

Relevant (include might be needed to be removed for now, since this eventually could break existing code)

lib/parser.js Outdated
})
}

if (selfClosing.indexOf(tag) > -1) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Relevant

lib/parser.js Outdated

if (!buffer.length) {
// undefined for selfClosing tags
if (node) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Relevant

@michael-ciniawsky
Copy link
Member Author

The approvals/lgtm check should be removed at best, since the service is offline for a while now and superseded by CODEOWNERS anyways :)

@michael-ciniawsky
Copy link
Member Author

options.tags => options.singleTags to be consistent with posthtml-render ?

expect(parser(options)(fixture)).to.eql(expected)
})

it('selfClosing', function () {
Copy link
Member

Choose a reason for hiding this comment

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

What is being tested?

Copy link
Member Author

Choose a reason for hiding this comment

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

#21 (diff), but I need to revisit this PR one more time (today) anyways, currently working on finishing/updating posthtml/posthtml-include#16 so this can finally land soon :)

@michael-ciniawsky michael-ciniawsky changed the title feat(parser): add tags option (options.tags) feat(parser): add support for custom selfClosing tags Dec 26, 2017
@michael-ciniawsky
Copy link
Member Author

michael-ciniawsky commented Dec 26, 2017

@gitscrum I removed the SELF_CLOSING tag list and the additional option since this is entirely unneeded to support custom selfClosing tags with the current parser. Could you review and verify before I squash the commit please ? Squashed :)

@laosb
Copy link

laosb commented May 2, 2018

Any progress?

@Scrum
Copy link
Member

Scrum commented May 4, 2018

@laosb Hi, something specific interests you from this PR ?

@andrewbanchich
Copy link

@Scrum Can this be fast tracked please? Parcel Bundler uses posthtml-parser and as a result anyone using it with self-closing tags has completely broken builds with no workarounds.

@Scrum
Copy link
Member

Scrum commented Jul 19, 2018

@andrewbanchich Hi, describe the case that breaks your build

@andrewbanchich
Copy link

Any tags that are self-closing don't output correctly. For Vue components, I'll often have <CustomElement /> since I don't need to add anything between.

@Scrum
Copy link
Member

Scrum commented Jul 20, 2018

What is the incorrectness?

@andrewbanchich
Copy link

Looks like this setting fixed it: parcel-bundler/parcel#1103 (comment)

Thanks though!

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.

4 participants