Skip to content

Conversation

@charpeni
Copy link
Contributor

@charpeni charpeni commented Feb 2, 2016

Related to #3.

Copy link
Contributor

Choose a reason for hiding this comment

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

why attach everything to the class? what's the advantage over just using var outside of the constructor?

@johannhof
Copy link
Contributor

Basically see my first comment. I personally really don't like the notion of just dumping everything into the constructor, especially considering that that code is potentially gonna be run many more times than if it were outside of the constructor.

Is there some important reason for doing this that I'm just missing here? Is this at least defined in some official style guide?

@davidLeonardi
Copy link
Contributor

Valid point. Sure, this can be improved. Ill work on it today.
On Tue, 2 Feb 2016 at 09:02, Johann Hofmann notifications@github.com
wrote:

Basically see my first comment. I personally really don't like the notion
of just dumping everything into the constructor, especially considering
that that code is potentially gonna be run many more times than if it were
outside of the constructor.

Is there some important reason for doing this that I'm just missing here?
Is this at least defined in some official style guide?


Reply to this email directly or view it on GitHub
#22 (comment)
.

davidLeonardi added a commit that referenced this pull request Feb 2, 2016
Add PropTypes and some fixs
@davidLeonardi davidLeonardi merged commit 3edc160 into react-native-simple-router-community:master Feb 2, 2016
@charpeni
Copy link
Contributor Author

charpeni commented Feb 2, 2016

I think that everything that is used only by the object should be defined as properties of this object.

Maybe we should define it like I did with propTypes property? Const before class, link as property after.

On my side, I don't like when everything is declared as const outside of the object without being link with this. But if you prefer something else, I will follow.

@davidLeonardi
Copy link
Contributor

I kind of am on the fence on this as well. The implementation might change
soon.

The problem is that ES6 does not allow static properties, thus forcing you
to put stuff into the constructor.

I'd advocate that this.firstRoute should indeed be a class property, but
that the stylesheets could very well be imported from another module
(that's the way i have that) and as such can be a CONST.

Furthermore, regarding the order of things in a file, and assuming 1 class
per file:

imports

constants

export class definition

within the classes: constructor, and lifecycle methods from bootstrap to
destruction, in that order, ending with a method for setting up events or
similar
within the constructor: this.state definition, compositions if needed, and
other class property initialisation to their default values.
within the classes: public API
within the classes: event handlers
within the classes: private API

mixins


A constant IMO should be something that can be seen on the same level as an
imported module, and should not be a class property itself. For instance,
style should be either a constant or an imported module. The definition of
routes should not, as it's not something that can be tucked away in a
separate module easily and is class specific.
Furthermore, if something can be set, it DEFINITELY is a class attribute.
but that applies to non--react mostly. :)

On Tue, Feb 2, 2016 at 1:59 PM Nicolas Charpentier notifications@github.com
wrote:

I think that everything that is used only by the object should be defined
as properties of this object.

Maybe we should define it like I did with propTypes property? Const before
class, link as property after.

On my side, I don't like when everything is declared as const outside of
the object without being link with this. But if you prefer something
else, I will follow.


Reply to this email directly or view it on GitHub
#22 (comment)
.

@johannhof
Copy link
Contributor

I think that everything that is used only by the object should be defined as properties of this object.

Ironically, you're actually exposing everything that the object is using to everyone with access to that class. Before, "private" variables were contained within the module scope.

@johannhof
Copy link
Contributor

But yeah I actually don't care about the style question, it just felt kind of wasteful to put the style initialization into the constructor.

@charpeni
Copy link
Contributor Author

charpeni commented Feb 2, 2016

@SEthX Go for that.

@johannhof You're right about stylesheets, it should be a const before class definition. I didn't think about the style initialization each time the class is constructed. Thanks for this clarification.

you're actually exposing everything that the object is using to everyone with access to that class.

I know that, but I think it's more cleaner to do that for specific properties to a class (As @SEthX said).

I hope this proposal will emerge soon https://github.com/jeffmo/es-class-fields-and-static-properties.

@charpeni charpeni deleted the issue-3 branch March 1, 2016 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants