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

ES6 syntax and eslint config update #19

Merged
merged 13 commits into from Feb 15, 2017

Conversation

Projects
None yet
3 participants
@lamartire
Copy link
Contributor

commented Feb 12, 2017

No description provided.

@lamartire lamartire referenced this pull request Feb 12, 2017

Closed

Move to ES6 #18

@ai

This comment has been minimized.

Copy link
Member

commented Feb 12, 2017

Could you rebase your PR to logux/eslint-config-logux master, because @frsv already updated it to ES6.

Also we need a addition rule to force arg => …, instead of (arg) => …. @frsv maybe you know it and could send PR to config?

@frsv

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2017

@ai

This comment has been minimized.

Copy link
Member

commented Feb 12, 2017

@frsv great :D

Konstantin Epishev
@ai

This comment has been minimized.

Copy link
Member

commented Feb 12, 2017

@lamartire don’t forget to fix all Travis CI issues ;)

@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2017

@ai can you update npm-package of logux/eslint-config-logux?

@ai

This comment has been minimized.

Copy link
Member

commented Feb 12, 2017

I plan to release it after we will be sure in it (move Logux Server to it).

Why you need a npm release?

@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2017

@ai after installig dependencies I'll take old version and eslint not worked.
Or can I install it manually from repository?

@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2017

Add some rules for templates -- logux/eslint-config#5
There were only problems with templates.

lamartire added some commits Feb 12, 2017

lamartire
lamartire
@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2017

@ai, need some help with tests.
Async test broken:

Tried solutions from docks, but it did not help.

@ai

This comment has been minimized.

Copy link
Member

commented Feb 12, 2017

Yarn has a problem with updating package from git. Just run rm -R node_modules && yarn cache clean && yarn to be sure. In Travis CI I just clean cache and run task again.

@ai

This comment has been minimized.

Copy link
Member

commented Feb 12, 2017

Async task is broken because you change it. Don’t use done in Jest, just return a Promise.

lamartire
@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2017

New problems.
SyntaxError: Block-scoped declarations (let, const, function, class) not yet supported outside strict mode, need update config again, besause we use 'strict': ['error', 'global'].

@lamartire lamartire referenced this pull request Feb 13, 2017

Closed

Remove strict rule #6

@ai

This comment has been minimized.

Copy link
Member

commented Feb 13, 2017

SyntaxError: Block-scoped declarations (let, const, function, class) not yet supported outside strict mode is not from ESLint, it is directly from node.js ;)

@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2017

So, I must remove all rules with let, const and keep all "var-rules"?

@ai

This comment has been minimized.

Copy link
Member

commented Feb 13, 2017

Yeap, replace let and const back to var.

@ai

This comment has been minimized.

Copy link
Member

commented Feb 13, 2017

Don't change a docs. Developers mostly will run server on latest node. And developers stocked on node 4 know about syntax limitation. This is why I prefer to force node 4 users to convert syntax from docs example, rather than force node 6 users to convert syntax to latest ES2016.

@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2017

I think the remaining errors relate to arrow functions. @ai, does it possible reason?

@ai

This comment has been minimized.

Copy link
Member

commented Feb 13, 2017

Also there is very a euse typo. It should be use.

@ai

This comment has been minimized.

Copy link
Member

commented Feb 13, 2017

Yeap. All Jest test are executed in parallel. So I used this.app to set current server for each year separated. Good question how to deal with it without this. If you have no ideas too, ask Jest author.

@ai

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

Just take some rare numbers.

lamartire
@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2017

I fixed many issues with ports.
There are still some problems, which are reproduced locally on master.

@ai

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

How do you stop server in afterEach without a this?

@ai

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

With function () { } this in tests means current test context. With () => it means global context.

So you have all this problems, because you didn't stop server after a test.

This is why we should write to Jest authors and ask them about API to access to test context with ES6 arrow functions.

@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2017

Clear.
Do you mind if I quote the comment about arrows in jest issue?
But on the one hand, I can try solve problem with save context in variable. Like @frsv advised.

@ai

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

Could you show a that variable based solution here in small example (I don't have laptop right now)

@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2017

I'm not sure, but:

var ctx;

beforeEach(function() {
  var ctx = this;
})

//...

afterEach(function() {
  if (ctx && ctx.server) {
     ctx.server.close()
     ctx = null;
  }
})
@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2017

@ai

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

Every test executed in parallel in Jest. This context variable in global, so it will not work 😉 It is just finally variable, beforeEach didn't do anything.

@frsv

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2017

@ai but why it works locally? In the end of each test server shuts down.

@ai

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

@frsv concurrency is tricky thing. Anyway current code is wrong, we need to fix this. Please, ask Jest authors and post a link to issue here.

@frsv

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2017

Well, @lamartire has created issue already – facebook/jest#2883

@ai

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

@frsv great!

@lamartire also don’t forget to mention somebody from core team. It increase response time.

@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2017

@ai thank for advice 😸

@ai

This comment has been minimized.

Copy link
Member

commented Feb 15, 2017

OK, seems like task doesn’t go in parallel anymore. Let’s replace this.app to just app as in comment facebook/jest#2883 (comment)

And them I will see a issues.

@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2017

Another way - facebook/jest#2883 (comment)
How about ignore this test and use it unmodified if replacing this.app by app will fail?

@ai

This comment has been minimized.

Copy link
Member

commented Feb 15, 2017

Replace this.app to app and then I will look and fix them.

@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2017

I think I can try resolve remaining issues myself today.
If not, I write here.

@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2017

Ha, all tests passed! 😸

@ai

This comment has been minimized.

Copy link
Member

commented Feb 15, 2017

Wow! Great.

@ai

This comment has been minimized.

Copy link
Member

commented Feb 15, 2017

@frsv @lamartire I plan to split cult of martians task to two to mark both of you as winners. Is it OK?

@ai

This comment has been minimized.

Copy link
Member

commented Feb 15, 2017

@lamartire I need your VK and Twitter account names

@ai ai merged commit e17bb14 into logux:master Feb 15, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@ai

This comment has been minimized.

Copy link
Member

commented Feb 15, 2017

@lamartire you still miss this and () => :D read about arrow-functions more careful.

This code will not work as you expected:

beforeEach(() => {
  var testId = shortid.generate()
  this.currentTest = testId
  this.tests[testId] = {}
})

this will be equal to global. So there is not point to have this.tests. You could do it simpler:

var currentTest
var tests = { }

beforeEach(() => {
  currentTest = shortid.generate()
  tests[testId] = {}
})
@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2017

All right. Thanks @ai! :)
I have not twitter, can give you my VK by email.

@ai

This comment has been minimized.

Copy link
Member

commented Feb 15, 2017

@lamartire this VK is needed to announce. If you want to keep it private I can just mentioned you by name :)

@lamartire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2017

I have empty twitter and little more filled vk. Let me be just Kostya, before another time when my skills will be better 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.