Ported to ES6 and rewrote some duplicate code #17

rafaelklaessen · 2016-11-07T20:25:35Z

Ported to ES6 and rewrote some duplicate code. See commits for more details.

Ported to ES6. TODO: improve code quality.

Fixed bugs and removed some duplicate code. I've tested it and all commands were working properly.

Fixed bugs and removed some duplicate code. I forgot to include these files as I edited them while writing the commit which unchecked them (but I didn't notice that).

Improved error checking. This means that if an error occurs, you can do cb({success: false, msg: error}). The error will be logged. If code succeeds, you can do cb({success: true}). Also fixed file checking. Try/catch is no longer required in order to determine if a file exists or not (therefore, I could rewrite some code). Also fixed that some files weren't actually checked because react.js or .js wasn't added after the name. Now they are actually checked. Also improved checking if .reactclirc exists. If it doesn't, an error will be shown.

Rewritten if/else blocks. First they were if else blocks with a try/catch block in them. Now they are a try/catch block with a if/else block in them. Also fixed a bug in view.js that would crash the scaffolder if the directory that client is set to (in .reactclirc) doesn't exist. Also fixed thata functions were actually arrow functions in config.js, checkspaces.js and checkduplicates.js. Removed unneeded require from test/generate.js.

dhruvdutt · 2016-11-08T11:06:59Z

bin/initialize.js


-const program = require('commander');


Why using var here? 'const` seems fine as far as it's not redefined anywhere else in the module.
You can read more about it here.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const

This is a compiled file, ES6 files are in src/.

dhruvdutt · 2016-11-08T11:08:03Z

bin/initialize.js

-				else {
-					setTimeout(() => {
+				} else {
+					setTimeout(function () {


Any special reason for removing the arrow functions?

This is a compiled file, ES6 files are in src/

dhruvdutt · 2016-11-08T11:10:03Z

bin/initialize.js


-						for(let count=0; count<numberOfPropTypes; count++) {
-							let propTypeChoice = {
+						for (var count = 0; count < numberOfPropTypes; count++) {


Using var here would make the variable count exposed outside the for loop which should not happen. Please consider using let in such cases.

This is a compiled file, ES6 files are in src/.

dhruvdutt · 2016-11-08T11:11:15Z

lib/generate.js

+
+		/**
+   * createComponentFacade
+  	 * @param {string} module - module name to create a component


Please indent properly.

This is a compiled file, ES6 files are in src/. This is caused by the compiler.

dhruvdutt · 2016-11-08T11:11:51Z

lib/generate.js

+   * createComponentFacade
+  	 * @param {string} module - module name to create a component
+   * @param {string} componentName - component name
+   * @param {string} asnwers - options provided when creating component


Typo in answers.

This typo was already here and I didn't see it, my bad.

dhruvdutt · 2016-11-08T11:17:52Z

src/lib/init.js

+		/**
+		 * promiseShortcut() function
+		 * Helper method. Created in order to shorten 2 almost the same promise calls.
+		 * @param {init} t Reference to this. Won't work if you just use this, you have to get the reference from a function parameter.


What is {init}? There is no such data type in JavaScript.
You can consider using null or object as a data type for this reference.

Init is the current instance of the init class. Normally, this would be this.

rajikaimal

If we have ES6 code under src, there is no use in adding compiled files under source control (git). Therefore let's remove compiled js files and add a npm command to compile ES6 code before publishing to npm registry.

rajikaimal · 2016-11-08T06:10:41Z

bin/initialize.js


-const program = require('commander');


Is there any reason for changing const / let to var ?

rafaelklaessen · 2016-11-08T15:08:29Z

Yeah that's true. But as we're using react (es6), does compiling make sense at all?

rajikaimal · 2016-11-08T15:31:54Z

Compiling makes sense, but maintaining compiled files doesn't. So let's go head with the suggestion shall we ? Any other suggestions ?

rafaelklaessen · 2016-11-08T16:43:41Z

Well as said, we are generating reactJS components that use ES6 as well (without any babel config), and we're targetting NodeJS v6 or higher, which supports ES6, so why does compiling make sense?

dhruvdutt · 2016-11-08T17:21:48Z

Please take a look at this table before removing the compiled ES5 code.
It seems NodeJS v6 doesn't support ES6 completely.
http://node.green/

rafaelklaessen · 2016-11-08T17:45:28Z

NodeJS v6 seems to support ES6 for 99%. The only unsupported things (2) aren't being used in the ES6 code. I think it's best to remove the compiled code. Shall we do this?

dhruvdutt · 2016-11-08T17:52:24Z

@rafaelklaessen Sure! But, I think @rajikaimal would make the final call. And we should keep in mind about that 1%. I can help adding gulp task (if needed) or current npm scripts are fine too.

rajikaimal · 2016-11-08T17:55:55Z

What I'm concerned is not about ES6 compatibility, I want to clarify why we include compiled/transpiled files into our repository. We can do the process of compilation/transpilation while we are testing this tool or adding new version to npm registry. Any comments ?

rajikaimal · 2016-11-08T17:56:56Z

I'm assuming that src contains all ES6 code so that it gets transpiled through babel to dist folder

rafaelklaessen · 2016-11-08T18:12:47Z

No, I'm just compiling everything from ./src to ./ with Babel

rafaelklaessen · 2016-11-08T18:18:22Z

@dhruvdutt Yea, that 1% that's unsupported is just 2 things that I've never heard of, so I'd say it's safe to just use ES6. And once again, ReactJS itself uses ES6 as well.

@rajikaimal The compiled files are included because I didn't want to mess up stuff with the original folsers. In the way it is now, it just works as before. (Btw: I've tested the tool by putting it in my global node_modules folder as npm test didn't test all code.) If we decide to keep the compiling process, we should indeed remove the compiled files and add them when testing or putting it on npm.

rajikaimal · 2016-11-10T16:04:57Z

So if we keep compiled files in our repository it means something like, keeping compiled java file .class files in repository in the case of a java project, right ?

rafaelklaessen · 2016-11-10T16:19:30Z

Kinda, the point is that we don't have to compile it (but you do have to compile Java).

rajikaimal · 2016-11-10T16:21:07Z

Alright so, if we have ES6 code now, we do have to compile for sure right ?

rafaelklaessen · 2016-11-10T16:21:28Z

I'd say that we'd just skip the whole compiling thing as nodeJS v6 supports ES6 almost completely.

rafaelklaessen · 2016-11-10T16:22:15Z

You see, it compiles to ES5, but it's just not necessary.

rajikaimal · 2016-11-10T16:22:35Z

Okay, so one question, do you have any compiled files here ES3 / ES5 in this PR

rafaelklaessen · 2016-11-10T16:23:39Z

Yes, in the current PR everything from ./src is compiled to ./. I think that we should put everything that's currently in ./src to ./ and remove the babel config.

rajikaimal · 2016-11-10T16:26:02Z

If we remove babel it will remove backward compatibility. So the point is to write code in ES6 and compile the source into ES5 for backward compatibility.

rafaelklaessen · 2016-11-10T16:27:07Z

Yes, but we're talking about a scaffolder that generates ES6 for ReactJS.

rafaelklaessen · 2016-11-10T16:27:51Z

So, why wouldn't a scaffolder that generates ES6 be written in ES6 as well?

rajikaimal · 2016-11-10T16:28:35Z

I'm referring to the source code of this tool itself not the code that this tool generates.

rafaelklaessen · 2016-11-10T16:29:09Z

I know, but I mean, using ES5 for a tool that generates ES6 doesn't make sense.

rafaelklaessen · 2016-11-10T16:29:36Z

As the user that uses it has to have node v6 or higher anyways.

rajikaimal · 2016-11-10T16:31:03Z

Like I mentioned earlier we need backward compatibility.

rajikaimal · 2016-11-10T16:31:40Z

For an example if a developer is using this tool on Node v4.

rafaelklaessen · 2016-11-10T16:32:09Z

If a developer is using Node v4 he can't use the generated React code either, so the tool's still useless.

rafaelklaessen · 2016-11-10T16:32:37Z

So the tool'd work, but he still wouldn't be able to use it.

rajikaimal · 2016-11-10T16:34:13Z

The code generated by this tool again should be compiled to ES5 through babel.

rafaelklaessen · 2016-11-10T16:34:36Z

But why aren't we adding a babel config for it then?

rajikaimal · 2016-11-10T16:35:57Z

Take a look at here.
We are using webpack, and it's loader for babel.

rafaelklaessen · 2016-11-10T16:37:02Z

Okay, let's remove the compiled files then.

rafaelklaessen · 2016-11-10T16:37:39Z

How are we gonna let it autocompile on test or when putting on npm registery?

rafaelklaessen · 2016-11-10T16:38:30Z

And how are we going to do that? Merge this PR and then add the autocompile stuff or...?

rajikaimal · 2016-11-10T16:41:18Z

Like I said, lets add the prepublish command on package.json, so that it will compile before publishing a new version to npm registry.
You can still commit to the same branch and it'll automatically update this same PR.

rajikaimal · 2016-11-10T16:42:23Z

I created a separate branch on this repo called es6 can you create a PR on that instead. Let's add es6 stuff there, before we go for a stable release.

rafaelklaessen · 2016-11-10T18:50:37Z

Okey, but just to be sure, you do realise that at the moment, it's compiling to ES5, and that the only thing we need to do is to remove the compiled files (and create prepublish commands etc)?

rajikaimal · 2016-11-11T05:18:20Z

Exactly, so let's go ahead with that plan shall we ?

rafaelklaessen · 2016-11-13T11:08:37Z

Yes, I'll create a pull request.

rajikaimal · 2016-11-13T16:26:04Z

Moved this PR to es6 branch with PR #19

rafaelklaessen added 5 commits November 4, 2016 23:24

Ported to ES6

a7a8960

Ported to ES6. TODO: improve code quality.

Fixed bugs and removed some duplicate code

5a76093

Fixed bugs and removed some duplicate code. I've tested it and all commands were working properly.

Fixed bugs and removed some duplicate code

ae08a1b

Fixed bugs and removed some duplicate code. I forgot to include these files as I edited them while writing the commit which unchecked them (but I didn't notice that).

rajikaimal mentioned this pull request Nov 8, 2016

es6-port #12

Open

dhruvdutt reviewed Nov 8, 2016

View reviewed changes

rajikaimal reviewed Nov 8, 2016

View reviewed changes

bin/initialize.js

const program = require('commander');

Copy link

Contributor

rajikaimal Nov 8, 2016

Choose a reason for hiding this comment

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

Is there any reason for changing const / let to var ?

rafaelklaessen closed this Nov 13, 2016

Ported to ES6 and rewrote some duplicate code #17

Ported to ES6 and rewrote some duplicate code #17

Conversation

rafaelklaessen commented Nov 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajikaimal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelklaessen commented Nov 8, 2016

rajikaimal commented Nov 8, 2016

rafaelklaessen commented Nov 8, 2016

dhruvdutt commented Nov 8, 2016 • edited Loading

rafaelklaessen commented Nov 8, 2016

dhruvdutt commented Nov 8, 2016

rajikaimal commented Nov 8, 2016

rajikaimal commented Nov 8, 2016

rafaelklaessen commented Nov 8, 2016

rafaelklaessen commented Nov 8, 2016 • edited Loading

rajikaimal commented Nov 10, 2016 • edited Loading

rafaelklaessen commented Nov 10, 2016 • edited Loading

rajikaimal commented Nov 10, 2016

rafaelklaessen commented Nov 10, 2016

rafaelklaessen commented Nov 10, 2016

rajikaimal commented Nov 10, 2016 • edited Loading

rafaelklaessen commented Nov 10, 2016

rajikaimal commented Nov 10, 2016

rafaelklaessen commented Nov 10, 2016

rafaelklaessen commented Nov 10, 2016

rajikaimal commented Nov 10, 2016

rafaelklaessen commented Nov 10, 2016

rafaelklaessen commented Nov 10, 2016

rajikaimal commented Nov 10, 2016 • edited Loading

rajikaimal commented Nov 10, 2016

rafaelklaessen commented Nov 10, 2016 • edited Loading

rafaelklaessen commented Nov 10, 2016

rajikaimal commented Nov 10, 2016

rafaelklaessen commented Nov 10, 2016

rajikaimal commented Nov 10, 2016 • edited Loading

rafaelklaessen commented Nov 10, 2016

rafaelklaessen commented Nov 10, 2016

rafaelklaessen commented Nov 10, 2016

rajikaimal commented Nov 10, 2016

rajikaimal commented Nov 10, 2016

rafaelklaessen commented Nov 10, 2016

rajikaimal commented Nov 11, 2016

rafaelklaessen commented Nov 13, 2016

rajikaimal commented Nov 13, 2016

dhruvdutt commented Nov 8, 2016 •

edited

Loading

rafaelklaessen commented Nov 8, 2016 •

edited

Loading

rajikaimal commented Nov 10, 2016 •

edited

Loading

rafaelklaessen commented Nov 10, 2016 •

edited

Loading

rajikaimal commented Nov 10, 2016 •

edited

Loading

rajikaimal commented Nov 10, 2016 •

edited

Loading

rafaelklaessen commented Nov 10, 2016 •

edited

Loading

rajikaimal commented Nov 10, 2016 •

edited

Loading