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

Add --es6 option to enable ES6 compliant syntax #200

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

Conversation

danielkhan
Copy link

@danielkhan danielkhan commented Jun 17, 2018

This PR

  • adds a new --es6 flag to enable ES6 (ES2015) syntax
  • replaces const with var if --es6 is set
  • adds string templates if --es6 is set
  • prefers arrow functions for callbacks if --es6 is set
  • makes sure that this feature is only available for Node.js >= 6.0.0
  • adds tests for --es6

Reference Link #167

const instead of var and template literals instead of concatenation.
@danielkhan danielkhan mentioned this pull request Jun 17, 2018
@danielkhan danielkhan closed this Jun 17, 2018
@danielkhan danielkhan reopened this Jun 17, 2018
@danielkhan danielkhan closed this Jun 17, 2018
@danielkhan danielkhan reopened this Jun 17, 2018
@danielkhan danielkhan closed this Jun 18, 2018
@danielkhan danielkhan reopened this Jun 18, 2018
@danielkhan danielkhan closed this Jun 18, 2018
@danielkhan danielkhan reopened this Jun 18, 2018
@danielkhan danielkhan closed this Jun 19, 2018
@danielkhan danielkhan reopened this Jun 19, 2018
@danielkhan danielkhan closed this Jun 19, 2018
@danielkhan danielkhan reopened this Jun 19, 2018
@danielkhan
Copy link
Author

Is there a chance to land this one? I'll be creating an online course in August and ideally I'd like to use the new feature there.

@alieslamifard
Copy link

@danielkhan it's better to validate your code with eslint(airbnb-es6). there is some issue with app.js and www.js file.

@danielkhan
Copy link
Author

@alieslamifard are you referring to the result? Right now, the resulting code is not fully airbnb compatible as the non-es6 code wasn't as well. To not fix too much within one PR, this wasn't done deliberately.

@koushikmln

This comment has been minimized.

@surajrana52

This comment has been minimized.

@robvaneck

This comment has been minimized.

@danielkhan

This comment has been minimized.

@omerl13

This comment has been minimized.

@carlosveloso

This comment has been minimized.

@dougwilson

This comment has been minimized.

@carlosveloso

This comment has been minimized.

@omerl13

This comment has been minimized.

@dougwilson

This comment has been minimized.

@yardenshoham

This comment has been minimized.

@dougwilson

This comment has been minimized.

@yardenshoham
Copy link

Yes, I have not gotten around to getting all the 4.17 stuff sorted out in the generator yet. Having PRs like thus one with merge conflicts also slows this down, as I want to get all these things into said 4.17 release so they do not wait again for who knows how long. Not having merge conflicts that I have to resolve is always a big help, though, if someone would like to resolve them on this PR.

How would one be able to resolve the conflicts? I'm new to this and can't find the conflicts (only that they're in test/cmd.js).

@dougwilson
Copy link
Contributor

Likely only OP or myself can. I will do it when I get around to it, of course. This is just one of the many things on my todo list and I'm not even in my own country right now to do any work anyway.

@danielkhan
Copy link
Author

@dougwilson - I think it's hard to prevent conflicts when PRs are sitting for such a long time.
I just resolved the conflict. Let me know if I can help.

@dougwilson
Copy link
Contributor

Thank you. Looks like just need to run "npm run lint" to fix the linting errors and it should be good to go, as that is the only thing failing on CI.

@dougwilson
Copy link
Contributor

There also seems to be a stray error: unknown option '--es6' printed during the tests if it is possible to hide that.

@dougwilson
Copy link
Contributor

P.S. I wanted to add that I am just listing out the things I would look into / modify when landing the PR. You are not required to do anything, as I will do it when landing. Just wanted to state that because I have had people rage close their PR when I was just trying to list the things I would do since they wanted it to land faster. I would prefer that did not happen here and if you feel like not making further changes I will do them no problem.

@danielkhan
Copy link
Author

@dougwilson - fixed linting problems. Console output is gone.

@backendsuraj
Copy link

When will this PR get merged with master?

@dougwilson
Copy link
Contributor

Ok, I am locking this as apparently people are going to keep asking the same question endlessly.

@expressjs expressjs locked and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.