Cleanup

[jridgewell]

Whew, this got a little too big. Then @thejameskyle made it even bigger. :stuck_out_tongue:

1. Misc cleaning (1f9dc2954b44f7a8f9c9169f213ac9dc9b19f866, b5e453cdfa9454851b061562c6c759d706a9ff49, ceab836401641b14d47389e2ae9489aa4735cc9f, 2c9397e71193bee4557ad10c516acc4dcf20cf6e)
2. Bugfix: Don't text() Identifiers and MemberExpressions (eb1b72cbf6aee5be6dcbdfdda7f6f20221a1cac4)
   • In React, they can be text or other Elements. Treating them as always text isn't the correct thing to do.
   • This open up the possibility of getters invoking iDOM functions, etc
3. Bugfix: Ensure self closing elements with spread attributes close (bb46b445481a6a246a9daf61e136762051feafdc)
4. Don't push unneeded arguments onto elementOpenStart (205df8455a5064b6f2d785f43e71797726f930e0)
5. Prevent unwanted function scopes from being created (afa2c79a5e072ca98859b820e7e50bc8b9508a11)

[jridgewell]

@thejameskyle: Would you mind reviewing this?

[jamiebuilds]

Looking now

[jamiebuilds]

Can we change some code style stuff in here as well:

1. Instead of building up and object, let's put all of the method of the visitor here: (the object made more sense when it was enter visitors`.
   • https://github.com/babel-plugins/babel-plugin-incremental-dom/blob/master/src/index.js#L72
2. Instead of destructuring can we call these node, makes it easier follow I think
   • https://github.com/babel-plugins/babel-plugin-incremental-dom/blob/master/src/index.js#L16
   • https://github.com/babel-plugins/babel-plugin-incremental-dom/blob/master/src/index.js#L53
   • https://github.com/babel-plugins/babel-plugin-incremental-dom/blob/master/src/index.js#L59
3. Instead of wrapping entire blocks in an if statement can we do the inverse and return early
   • https://github.com/babel-plugins/babel-plugin-incremental-dom/blob/master/src/helpers.js#L15
   • https://github.com/babel-plugins/babel-plugin-incremental-dom/blob/master/src/helpers.js#L128-L131 (continue)
   • https://github.com/babel-plugins/babel-plugin-incremental-dom/blob/master/src/helpers.js#L163-L174
4. If you decide to keep the helper builder function (re: https://github.com/babel-plugins/babel-plugin-incremental-dom/pull/5#discussion_r35489093), can you make all the functions exported the same way:
   • https://github.com/babel-plugins/babel-plugin-incremental-dom/blob/master/src/helpers.js#L106-L114
   • https://github.com/babel-plugins/babel-plugin-incremental-dom/blob/master/src/helpers.js#L121
   • https://github.com/babel-plugins/babel-plugin-incremental-dom/blob/master/src/helpers.js#L161

Sorry for the massive comment

[jridgewell]

1. https://github.com/babel-plugins/babel-plugin-incremental-dom/commit/f519f79edfc5428fc0e1d06b19acacfef4d02a13
2. https://github.com/babel-plugins/babel-plugin-incremental-dom/commit/f056fb3b1b507a6d4d01d2281f5e59ead4871a63
3. https://github.com/babel-plugins/babel-plugin-incremental-dom/commit/9354cd325195253a3aa488840606670e98c91c1c
4. https://github.com/babel-plugins/babel-plugin-incremental-dom/commit/a4f60ffdd3c6ec93b84015c39a1ca58a3d410b7c

[jamiebuilds]

Then @thejameskyle made it even bigger. :stuck_out_tongue:

I'm just bumping your commit count, that's all.

[jamiebuilds]

Looks good

[jamiebuilds]

btw @jridgewell, @sebmck mentioned to me earlier that he's going to be enforcing linting rules across plugins in this org in the future

[jridgewell]

https://github.com/babel-plugins/babel-plugin-incremental-dom/commit/eb1b72cbf6aee5be6dcbdfdda7f6f20221a1cac4 is a breaking change. V2?

---

enforcing linting rules across plugins in this org in the future

Is there are an lint configuration?

[sebmck]

@jridgewell

Is there are an lint configuration?

https://github.com/babel/babel/blob/master/.eslintrc

[jamiebuilds]

Re: eb1b72c

I'm now confused by this change, <div>{"Hello"}</div> is treated the same as <div>Hello</div> in React isn't it?

[jridgewell]

I'm now confused by this change, <div>{"Hello"}</div> is treated the same as <div>Hello</div> in React isn't it?

Literals and implied text (<div>hello</div>) are treated the same. That commit changes the following:

<div>{hello}</div>
<div>{obj.hello}</div>

Using the online compiler to test, hello and obj.hello can reference a React Element which will be inserted as a child. That doesn't make sense for iDOM, since you can't return an element to insert it — only calling one of the iDOM functions will do anything.

[jamiebuilds]

This seems like a pretty significant issue. Do you think there's any way for idom to support this case?

<div>{'text'}</div>

elementOpen('div')
renderArbitrary('text')
elementClose('div')

<div>{<div></div>}</div>

elementOpen('div')
renderArbitrary((elementOpen('div'), elementClose('div'))
elementClose('div')

Working alongside https://github.com/google/incremental-dom/pull/75

[jridgewell]

This seems like a pretty significant issue. Do you think there's any way for idom to support this case?

Without a pretty big renderArbitrary and a lot of edge cases, no it's not. google/incremental-dom#75 will get us some of the way, but that'll still return a fleshed DOM node. The helper would have to scrape it for the tagName, key, attrs, and recurse into the children.

You'd be prevented from using statics, because it wouldn't be possible to determine if the particular attribute will change in the future.

I'm not even sure if that covers all of the possibilities JSX does.

var node = (elementOpen('div', key, ['class', 'test', 'key', key], id, x), elementClosed('div');
console.log(node); // => <div key="123" class="test" id="abc"></div>

function renderArbitrary(node) {
    if (isElement) {
        // scrape tag, key, attributes, and recurse into children
    } else if (isTextNode) {
        text(node.data);
    } else if (isString) {
        text node.data;
    } else if (isObject) {
        _.each(node, renderArbitrary);
    }
};

I think this is an issue with the differences between the renderers (React v iDOM), not the JSX -> iDOM compilation.

[jamiebuilds]

Is there any way we can support the text case? This basically gets rid of the possibility of using dynamic text content minus using idom directly.

[jridgewell]

Is there any way we can support the text case?

Certainly, let me work on that.