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

Address vhtml#11: Add ability to directly set innerHTML of a component using the dangerouslySetInnerHTML attribute. #12

Merged
merged 5 commits into from
May 23, 2019

Conversation

pl12133
Copy link
Contributor

@pl12133 pl12133 commented Jul 21, 2017

Should take care of #11.

New Size: 630B.
New Size: 624B.
New Size: 615B.

50B is kinda significant 😢 but I consider this a useful feature.

This was initially a 50B increase in size but is now worked down to 35B increase in size.

…t using the `dangerouslySetInnerHTML` attribute.
@paranoidjk
Copy link

+1 for this

@developit
Copy link
Owner

I think we can slim this down a bit without losing much functionality, but inlining the property name and skipping one of the conditionals:

if (attrs && attrs.dangerouslySetInnerHTML) {
    s += attrs.dangerouslySetInnerHTML.__html;
}

@pl12133
Copy link
Contributor Author

pl12133 commented Sep 4, 2017

Good call on dropping the extra check, no need for __html to be valid to add it. I didn't inline the property because that seems to cost 1B more than declaring let setInnerHTMLAttr = 'dangerouslySetInnerHTML'.

However I did find 4B that can be cut out regardless (b8199d3) which should helps a little.

This allows us to assume that `attrs` is a plain object, and remove multiple guards.
Saves 6B.
@developit developit self-requested a review December 13, 2017 00:54
@developit developit self-assigned this Dec 13, 2017
@ithinkihaveacat
Copy link
Contributor

Aha, whoops maybe I should've looked here before implementing #17

#17 does pass one test that this doesn't, though I'm not sure whether it "should" according to the spec. The test is:

it('should support dangerouslySetInnerHTML (without __html)', () => {
  expect(
    <div dangerouslySetInnerHTML>foo</div>
  ).to.equal(
    '<div></div>'
  );
});

In this case, this PR returns <div>undefined</div>.

@developit
Copy link
Owner

@ithinkihaveacat - I'm going to merge Peter's here, since it's substantially smaller. I personally don't care what we return for the case of a boolean dangerouslySetInnerHTML value - since that actually throws in React most folks wouldn't be using it.

@developit developit merged commit 714c9e0 into developit:master May 23, 2019
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.

4 participants