Where does the `@element` come from in ReactEditorView?


#1

I added a comment on the diff that I believe introduces @element to ReactEditorView:

Specifically, I am curious about this line in ReactEditorView's constructor:

@component = React.renderComponent(EditorComponent(props), @element)

I don’t see where @element is defined. I also looked in the View superclass from SpacePen and don’t see anything there, either.

Basically, I am trying to leverage React in my Atom package, so I thought that I should treat ReactEditorView as the current best practice. From the blog post, it sounds like React will soon be supported properly, but for now, it seems like I have to shoehorn React into SpacePen.


#2

@element is the root element of the View, returned from @content. So for the ReactEditorView it corresponds to the <div class="editor react editor-colors"> created here:


#3

And I replied to it (9 hours ago):


EDIT
Since Discourse is insisting on stripping the anchor from my link, here’s my comment:

From here:
https://github.com/atom/atom/blob/a134a60ce83939b7bd906b89afbb85e557c7a7f1/src/space-pen-extensions.coffee#L72


#4

@thomasjo I’m sorry I missed your original reply. It looks like half of my GitHub email gets filed under the Gmail promotion tab, so I missed it. I dragged it to the Primary tab in Gmail, so hopefully it/I will learn.


#5

Also, thanks, the use of Object.defineProperty explains why grepping for @element failed to work for me.

How would I instantiate a SpacePen component such that the static content() method gets exercised? I’m trying to create my own View, which I instantiate via the new keyword, so I don’t think it exercises the content() method.

Actually, https://github.com/atom/space-pen/issues/30 seems to highlight this issue, as well. Should I worry about this? Is SpacePen going to continue to be a thing or will developers be able to build things purely in React soon enough?


#6

If you call the super constructor method in your constructor (or does not write a constructor at all) the content method will be called, then the initialize method. I frequently get trapped by this, even after years writing coffeescript… ^^


#7

I discovered that I am being bitten by a number of things. First, I should be clear that I am not writing in CoffeeScript: I am writing in ES6 using JSX as a transpiler. To learn, I am trying to build a React-based autocomplete widget, so I was modeling my work off of a combination of https://github.com/atom/autocomplete/blob/master/lib/autocomplete-view.coffee and https://github.com/atom/atom/blob/master/src/react-editor-view.coffee.

The first weirdness is the relationship between a SpacePen View’s constructor and its initialize() method. If you only have an initialize() method and you want to pass it args, you need to pass those args to your View subclass’s constructor for them to make it to initialize(). If you happen to implement a constructor later, then you need to call super() with all of those args. In CoffeeScript, this is a little easier to remember because super gets transpiled to View.__super__.initialize.apply(this, arguments), but super without parens is not recognized in ES6 (at least not by JSX), so you have to forward all args explicitly to the superclass constructor via super(arg1, arg2, etc.).

The next weirdness is space-pen-extensions. It feels weird/bad to require something for its side-effects. These two lines from https://github.com/atom/atom/blob/a134a60ce83939b7bd906b89afbb85e557c7a7f1/src/space-pen-extensions.coffee kill me:

Subscriber.includeInto(spacePen.View)
Object.defineProperty jQuery.fn, 'element', get: -> @[0]

The first made it hard to discover where @subscribeToCommand came from in autocomplete-view.coffee.
The second made it hard to discover where @element came from in react-editor-view.coffee.

Because these two lines of code extend the functionality of a class through non-standard methods, traditional grep techniques do not turn up these results.

Finally, I cannot find an example of how to require space-pen-extensions outside of Atom. In fact, react-editor-view.coffee appears to cheat, as it does:

{View, $} = require 'space-pen'

Since it is using @element, I would argue that this should be:

{View, $} = require './space-pen-extensions'

AFAICT, ReactEditorView only happens to work right now because the SpacePen extensions “happen” to have been loaded by some other module first. I would argue that this is more evidence that this load-a-module-for-its-side effects technique is dirty. I suspect that as the codebase grows (and as code is spread across more small Atom packages), these types of hidden side-effects are likely to bite more developers.


#8

No need to apologize, just wanted to make sure you saw it :smiley:


#9

Not sure why you refer to mixins and prototype decoration as non-standard way (since both are pretty common techniques in javascript since some time now). And I do not see grepping as a traditional technique neither, for years we had to protect for…in loops with hasOwnProperty calls because of prototype pollution, and that precisely why Object.defineProperty was introduced: to allow to define non-enumerable|non-writable properties.

But it sure makes the code harder to follow when you are not used to these practices.


#10

@abe I would say that they are non-standard because they violate the spirit of encapsulation. That is, I expect that I should be able to look at space-pen.coffee and find the comprehensive definition of the View class. If you allow the use of Object.defineProperty in your codebase, then the defined behavior of View could be spread out all over the place.

Now View's behavior cannot be determined in isolation: it is unpredictable because it depends on what other code is loaded at runtime. I think the fact that {View, $} = require 'space-pen' works in react-editor-view.coffee is evidence of this. Allowing this use of dynamic behavior will make the codebase harder to analyze, which will likely become of more interest as it grows.


#11

@bolinfest I agree it looks like a serious smell. It would have make more sense if the @element property was defined a writable property in space-pen and then overwritten using Object.defineProperty in Atom to make a read-only property, as space-pen can be used outside of Atom, not relying in Object.defineProperty ensure compatibility with IE < 9.
Another thing that I found smelly in this case is that it should be an enumerable property. I would never create non-enumerable or non-configurable accessors unless I touch the Object class directly.
This is why I prefer decorate the Function class to provide some helpers rather than using Object.defineProperty directly:

Otherwise I was mostly arguing that features injection and non-enumerable properties are common things in node and as a result Atom. So using for..in loops or Object.keys no longer offer guarantees over the results.

What is seriously lacking in regard with this last point, is a better support for mixins and class reopening in biscotto (but it looks like a tough job).