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

Conflict between `no-new` and `no-unused-vars` rules #958

Closed
synthecypher opened this issue Jul 25, 2017 · 7 comments

Comments

@synthecypher
Copy link

commented Jul 25, 2017

If I want to instantiate a class as part of an initialization process but I do nothing with the instance after the fact I run into the two dilemmas.

// no-unused-vars
const gameEngineInstance = new GameEngine()

If I remove the unused variable I get the following

// no-new
new GameEngine()

If I do the following it won't instantiate the class.

GameEngine()

So what should I do in this example to comply with standard?

@yoshuawuyts

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2017

@LinusU

This comment has been minimized.

Copy link
Member

commented Jul 31, 2017

I agree with what @yoshuawuyts said, it's very bad practice to have side effects happen in a constructor. I would recommend refactoring so that it isn't necessary.

If you do want to continue doing so, you can disable linting for that line, like so:

// eslint-disable-next-line
new GameEngine()

@LinusU LinusU closed this Jul 31, 2017

@synthecypher

This comment has been minimized.

Copy link
Author

commented Jul 31, 2017

@LinusU can you link me to some information or an example of how I might write such an API?

@bcomnes

This comment has been minimized.

Copy link
Member

commented Jul 31, 2017

Move the side-effects into a function or method call of an instance.

E.G. collect options on instantiation, then once instantiated, run a .mount() or .start() method that does whatever side-effects you were doing in the instantiation.

@LinusU

This comment has been minimized.

Copy link
Member

commented Jul 31, 2017

In this specific case, I can imagine that the GameEngine sets up some kind of loop that runs every frame or similar. Instead of doing that in the constructor move that to a .start() function.

e.g.

old:

class GameEngine {
  constructor () {
    // ...

    setInterval(() => this.renderFrame(), 1000 / 30)
  }

  // ...
}

// ...

new GameEngine()

new:

class GameEngine {
  constructor () {
    // ...
  }

  start () {
    setInterval(() => this.renderFrame(), 1000 / 30)
  }

  // ...
}

// ...

const engine = new GameEngine()

engine.start()
@synthecypher

This comment has been minimized.

Copy link
Author

commented Aug 1, 2017

@LinusU

This comment has been minimized.

Copy link
Member

commented Aug 1, 2017

The constructor it initialises variables with sensible defaults does that count as a side effect?

No, that is how it should be 👍

Side effects are something that affects anything outside of the class itself. So it's perfectly fine to modify anything that beings with this., but you shouldn't modify anything outside of the class.

setTimeout, setInterval, process.nextTick, .addEventListener, .requestAnimationFrame, etc. all implicitly modifies global state and we should always think really carefully before using them in a constructor.

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

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