shama / yo-yoify

Transform choo, yo-yo or bel template strings into pure and fast document calls
111 stars 17 forks source link

allow inline objects #55

Closed jongacnik closed 7 years ago

jongacnik commented 7 years ago

Took a stab at implementing inline objects re: #47

It seems like we can't know whether or not a variable is an object without doing partial code execution in the transform, so instead I delegated that check to the client. I'm not sure if this is the right way to do this, definitely open to feedback! Seems to be working well for my use cases though.

We can add to the namespaced setAttribute for svg as well if this is looking like an ok way to handle this.

bcomnes commented 7 years ago

Any way to test this?

jongacnik commented 7 years ago

I can write some tests (probably tomorrow). Will need to run a browser test to actually show setting attributes correctly.

goto-bus-stop commented 7 years ago

There are some edge cases to take into account: an object with { className: 'xyz' } should set the class="xyz" attribute, an { onclick: whatever } should do el.onclick= instead of setAttribute('onclick'). This is the function babel-plugin-yo-yoify uses if there's an unknown value in an <div ${attr}="value" /> or <div ${attr} /> position, I think it covers most bases:

https://github.com/goto-bus-stop/babel-plugin-yo-yoify/blob/e6a10afd01d5842ec15689477504222f5c6640db/addDynamicAttributeHelper.js

jongacnik commented 7 years ago

@goto-bus-stop ya, I knew it was going to need some specificity. That function looks great, thanks! Placed this in, seems to handle things well, and saves space since we're now just requiring this function rather than inlining checks everywhere.

Curious if there was a reason to use == rather than === for comparisons? == was causing tests to fail so I swapped out for strict.

goto-bus-stop commented 7 years ago

Sweet 🎉 Think the reason for == was to save bytes, since the types were always known anyway. After gzip that doesn't matter much so may not be worth it 🤷‍♀️ In babel-plugin-yo-yoify, it inlines that function in every file that needs it, so I tried to keep it as small as possible. When this gets merged I'll just change the plugin to point to yo-yoify/lib/setAttribute instead.

jongacnik commented 7 years ago

Added a browser test but we're failing in node 0.10

jongacnik commented 7 years ago

Updated tests to run in node 6 and 7

bcomnes commented 7 years ago

yo-yoify@3.8.0

bcomnes commented 7 years ago

unpublished because there were breaking changes that never got published

bcomnes commented 7 years ago

yo-yoify@4.0.0