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 test cases for spread attributes #52

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

ForbesLindesay
Copy link
Member

These currently already lex without any problems. I’d like to use these to replace &attributes and provide proper escaping in the process

@ForbesLindesay
Copy link
Member Author

This only works if it's the first attribute in the list, and doesn't work for more complex JavaScript expressions. It would be nice to adapt it to work more reliably.

@TimothyGu
Copy link
Member

This only works if it's the first attribute in the list, and doesn't work for more complex JavaScript expressions.

Yeah, since the dots might be part of a JavaScript expression (e.g. a(href=object .attr)). But since ... shouldn't be valid in normal JavaScript we can probably use a special case. Just thinking by talking.

@TimothyGu
Copy link
Member

Just implemented that. All tests pass.

@@ -0,0 +1,5 @@
div(...attrs)
div(...attrs=foo)
Copy link
Member

Choose a reason for hiding this comment

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

Just a question: what is the foo? Why couldn't one just do (...foo)?

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking was that it could mean you can namespace things: e.g.

div(...data=myDataAttributes)

compiled with:

{myDataAttributes: {foo: 'foo-val', bar: 'bar-val'}}

results in:

<div data-foo="foo-val" data-bar="bar-val"></div>

Probably this is the wrong syntax for that though. I think I would prefer to support something like:

div(data=...myDataAttributes)

or

div(data...=myDataAttributes)

as I think that reads better/more clearly. Probably best to remove support for having a "value" for spread attributes. If we make sure it's a syntax-error for now, we can always add it in later.

@ForbesLindesay
Copy link
Member Author

Awesome. One last thing before we merge this: We should define a new token-type of spread-attr which should have mustEscape: true (we can think about syntax for un-escaped spread attributes some other time) and val: String

@TimothyGu
Copy link
Member

So basically there are two classes of spread attributes:

  1. a(...likeAndAttributes)
  2. a(data=...specialSpreadAttributes)

I'll use spread-attribute for the first and spread-attribute-set for the second.

@ForbesLindesay
Copy link
Member Author

Sounds good. I'm inclined to only implement spread-attribute for now, and leave spread-attribute-set as an idea we can investigate in the future. We'll need to remove the bonus test case 👍

@ForbesLindesay
Copy link
Member Author

I'm labeling this as a breaking change because we currently allow attributes at the start to be like a(...foo='bar') and output <a ...foo="bar"></a>.

@TimothyGu
Copy link
Member

Yep, sounds about right.

@TimothyGu
Copy link
Member

Pushed a few commits that 1. added an unescaped version (div(...!unescapedAttrs)) and 2. also fixed the behavior of some regular attributes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants