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

lint: default to ecmaVersion: 6, keep ecmaVersion: 5 for client/ #593

Merged
merged 1 commit into from Sep 7, 2016

Conversation

williamboman
Copy link
Member

@williamboman williamboman commented Sep 5, 2016

Cleanest solution I could come up with right now.

Adds ~3s to the linting process though, ymmv. (not sure how big part the added es6 environment plays in that, doubt it's much)

Unblocks #592.

@williamboman williamboman force-pushed the chore/eslint-es6-src branch 2 times, most recently from c3334dc to a9341a9 Compare September 5, 2016 14:10
@astorije
Copy link
Member

astorije commented Sep 6, 2016

Awesome start!

Reason why it takes a bit longer to run is because you are loading ESLint "environment" (or whatever it's called) 3 times now.

I think this is a bit convoluted though IMO. I'd go for simpler: an ES6-enable one and another that it's not. Something like npm run lint:js:es3 that we run against client/ only, and npm run lint:js:es6 that we run against anything else.
How does that sound?

@astorije astorije self-assigned this Sep 6, 2016
@astorije astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Sep 6, 2016
@williamboman
Copy link
Member Author

Yeah that makes sense. Shaved off 1.5-2s.

@williamboman williamboman changed the title lint: enable es6 env for src/ lint: default to ecmaVersion: 6, keep ecmaVersion: 5 for client/ Sep 6, 2016
@AlMcKinlay
Copy link
Member

👍 looks good

@astorije
Copy link
Member

astorije commented Sep 7, 2016

Slightly off-topic but as an FYI in case it's useful someday, I discovered that setting up es6 in the env list sets up the ecmaVersion to 6 as well, even though it's not documented anywhere (well, for now...

Anyway, 👍 on this, cool!

@astorije astorije merged commit 4be6a6e into thelounge:master Sep 7, 2016
@astorije astorije added this to the 2.0.0 milestone Sep 7, 2016
@williamboman
Copy link
Member Author

williamboman commented Sep 7, 2016

Slightly off-topic but as an FYI in case it's useful someday, I discovered that setting up es6 in the env list sets up the ecmaVersion to 6 as well, even though it's not documented anywhere (well, for now...

Yep, that's how I did it first. #javascript on IRC told me --env es6 is being deprecated and that ES version should be set through parser options.

@williamboman williamboman deleted the chore/eslint-es6-src branch September 7, 2016 13:46
@astorije
Copy link
Member

astorije commented Sep 7, 2016

--env es6 is being deprecated and that ES version should be set through parser options.

Hmm, that's highly surprising (unless they were saying that --env is deprecated but env: es6 in the file is not). Note that when using actual ES6 features, such as Promise or other globals, env: es6 has to be specified. But we are not concerned about that for now :-)

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
lint: default to ecmaVersion: 6, keep ecmaVersion: 5 for client/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants