Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds the ability to generate a project using ES2015 template #176

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bluzi
Copy link

@bluzi bluzi commented Oct 21, 2017

Hi @dougwilson,

As you requested in my previous PR, I adapted my changes to the 4.16 branch and added tests.
I found it easier to just clone the 4.16 branch and make the changes again, since there were too many conflicts.

Anyway, this PR has everything there was in the previous PR, and also tests for --es2015 and --es5.

Let me know if there's anything else you need before you merge it :)

@bluzi
Copy link
Author

bluzi commented Oct 21, 2017

Seems like there are some linter warnings, I'll fix them soon. Feel free to look at the code in the meanwhile.

@bluzi
Copy link
Author

bluzi commented Oct 28, 2017

Weird, I can't reproduce the linting errors Travis shows on my local copy.
I ran npm run lint and it produced no errors. Also, all tests in npm run test-ci pass. (that's also the situation in Travis)

@dougwilson, I'm pretty sure my code is OK and ready to be merged.

@dougwilson
Copy link
Contributor

If you look at the diff on here you can see the linting issues, including that it seems like a lot of unrelated changes to your actual change are in this PR. The Github merge button is disabled until the CI is passing. The purpose of the CI runs is so that it can run in a known-good environment just in case there is something wrong locally. I can produce the same linting errors on my machine as I am seeing on Travis, so there may be somethig going on on your local. You can use the output from Travis CI to manually make the fixes, at least.

Alternatively I can help out by making them for you, but I will need to do this when I am at a computer and may be a bit, apologies.

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Just a couple comments and can you add a test for the autoderection functionality?

@@ -9,6 +9,7 @@ var program = require('commander')
var readline = require('readline')
var sortedObject = require('sorted-object')
var util = require('util')
var detect = require('feature-detect-es6')
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be alphabetized

package.json Outdated
@@ -28,6 +28,7 @@
"commander": "2.11.0",
"ejs": "2.5.7",
"minimatch": "3.0.4",
"feature-detect-es6": "^1.3.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to change to specific version instead of verion range.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also alphabetize

@@ -68,7 +76,7 @@ if (!exit.exited) {
* Install an around function; AOP.
*/

function around (obj, method, fn) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a lot of changes like this that don't seem related to yiur changes. In fact deleting the space is a linting error

@bluzi
Copy link
Author

bluzi commented Oct 28, 2017

Hey @dougwilson, you're right. Looking at the diffs, I see them too.
Must be vscode's auto-formatting

@dougwilson
Copy link
Contributor

P.S. I will help make the above changes if you don't get to it first; I wanted to list them in case you have time, otherwise no worries.

@bluzi
Copy link
Author

bluzi commented Oct 28, 2017

@dougwilson I've fixed all of the linting issues, as well as the other comments you had.
As to the auto detection test - Could you please elaborate what you had in mind? Travis runs the tests on multiple node versions, but I can't think of a way I could know which version of node is currently executing the test.

@imcodingideas
Copy link

@dougwilson is there anything I can do to help move this PR along?

@dougwilson
Copy link
Contributor

Just needs the test suite to pass in CI and tests added for the auto detection feature that was added in the PR 👌 Sorry I haven't been able to do this myself, but if someone is willing to that would help get this moving again without waiting for me 😄

@bluzi
Copy link
Author

bluzi commented Jan 16, 2018

Sorry, but I also don't have time to finish the PR at the moment. If anyone wants to jump in and fix the tests (it's the only thing that's left, don't think it'll take more than few hours tops), read the PR and create your on PR in my fork.

@dougwilson
Copy link
Contributor

I do plan to help finish the PR for you, just haven't gotten to it. It seems @imcodingideas is willing to help out 👌

@@ -1014,6 +1014,128 @@ describe('express(1)', function () {
})
})
})

describe('es2015', function () {
Copy link
Author

@bluzi bluzi Jan 16, 2018

Choose a reason for hiding this comment

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

@imcodingideas / @dougwilson, if you want to help:
The only issue I know about, is that the --es2015 flag test also runs in older Node environments in the CI.
Because older versions doesn't support es2015, the test fails when it tries to run the generated application, that obviously contains es2015 code. (as it was generated with --es2015 flag)

We should either:

  • Test es2015 support even when the --es2015 flag is presented, and show error message when a user tries to generate es2015 application on an environment where es2015 is not supported.
  • "Disable" this test if it runs on older Node versions, where es2015 is not supported.

If you'd like to help, feel free to discuss it and clear out what exactly should be done to solve this (@dougwilson), or just go ahead and implement it yourselves.
That said, I might jump in and just do it myself if I'll find the time :)

Thanks!

Choose a reason for hiding this comment

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

OK. I like the test es2015 support when you use the flag. I'm just seeing these messages. I've had a long month.

@bferris1 bferris1 mentioned this pull request Jan 23, 2018
@imcodingideas
Copy link

imcodingideas commented Feb 12, 2018

Just an update on this PR. I added a flag to check if the current node version supports these features with: detect.all('const', 'let', 'arrowFunction') in the beforeEach though it seems that this.skip() fails to skip tests with nested describe calls as you can see it's getting fixed here: mochajs/mocha#3225

@bluzi
Copy link
Author

bluzi commented Feb 12, 2018

Just make an if condition then IMHO

@imcodingideas
Copy link

imcodingideas commented Feb 12, 2018

@bluzi Im going to wrap this up today at work. This could have been an easy fix but the issue I was running into last night was because the nested describe calls. What I will do is just like you said, wrap everything inside the first before in the if. I can get truthly statement to work when I add the same skip feature inside the nested before and after. I'll come up with something today.

describe('es2015', function() {
  before(function() {
    if (detectFeature) {
      this.skip()
    }
  })

  it('blah blah', function(done) {
    // somethings here.
  })

  describe('npm start', function() {
    before(function() {
      // something here.
    })

    after(function() {
      // after
    })

    it('blah blah', function(done) {
      // somethings here.
    })
  })
})

@ZeroCho
Copy link

ZeroCho commented Feb 24, 2018

@imcodingideas Do you mean that mochajs/mocha#3225 needs to be fixed first to implement your code?
As this.skip() makes test fail?

BTW, I'm really looking forward to this PR being merged.

@dougwilson dougwilson changed the base branch from 4.16 to master March 15, 2018 22:29
@bencmbrook
Copy link

Was this PR abandoned? Would love to see updated syntax.

@iDhruv22
Copy link

Any update on PR?
Need to add "import" "export" instead of require().

@arnavb
Copy link

arnavb commented Jan 19, 2019

It's 2019 already. I'm also looking for this functionality; when will this PR be updated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants