13

Why do I get these errors?

Problem at line 329 character 60: Do not use 'new' for side effects.

new widget.StyledDropdown(dojo.byId("sTitle"));

Problem at line 330 character 61: Do not use 'new' for side effects.

new widget.StyledDropdown(dojo.byId("sSuffix"));

Problem at line 336 character 57: Do not use 'new' for side effects.

true,{shortenName : true,maxChars : 20});

Problem at line 338 character 129: Do not use 'new' for side effects.

new widget.StyledDropdown(dojo.byId("sCountry"),USPS.Address.countrySw...

James Allardice
  • 164,175
  • 21
  • 332
  • 312
Amen Ra
  • 2,843
  • 8
  • 47
  • 85

3 Answers3

23

You're not storing a reference to the newly-created objects, which is a code smell.

JSLint is saying "You're creating some objects but immediately discarding them; the only possible reason you can be doing that is that the act of creating the objects has side-effects, which is weird."

You can lose the warning either by preventing your constructors having side effects (which will mean finding some other way of doing whatever it is they're doing, eg. by moving that code into a normal function) or by storing references to the newly-created objects (even in a temporary local variable that you discard).

RichieHindle
  • 272,464
  • 47
  • 358
  • 399
  • If you means "code smell", then so do I, but it is unfortunately quite apt. – Tim Down Sep 10 '10 at 16:15
  • 3
    @Tim: Yup, that's the one I meant. Why can't we just say "poor practice"? – T.J. Crowder Sep 10 '10 at 16:17
  • What would you say about using the new to pass an instance of itself down into a callback after instantiating itself? – Tegra Detra Feb 29 '12 at 06:40
  • Solution examples would be nice. – maasha Sep 15 '16 at 16:46
  • 3
    I wonder about this one... is it bad practice to do `new Notification({title: title, body: body})` but good practice to do `var unused; { ... some code ... }; unused = new Notificaiton({title: title, body: body})`? Seems to me that storing the ref in a temp var is also bad and confusing. Also wouldn't this be another jslint error (unused var): `function blah (args) { var x = new Notification(args); /* more code */ }`? – Kasapo Jan 24 '17 at 21:24
  • @T.J.Crowder 'code smell' is more apt than 'poor practice', because it the code in question _may_ indicate poor practice and further investigation would be in order. Sometimes the 'smell' may be a false alarm, or something to live with for say performance reasons. – Adam Tolley Feb 13 '17 at 17:36
3

Rethinking the strategy is best, but more often, handling tech debt during a development cycle isn't an option.

If you are using JSHint you can override this option on a case by case basis. Add this jshint comment in the scope of the offending code.

/* jshint -W031 */
new widget.StyledDropdown(dojo.byId("sTitle"));
new widget.StyledDropdown(dojo.byId("sSuffix"));
...

Inline configurations are function scoped. So anything outside the scope of the comment will still be checked.

Shanimal
  • 11,517
  • 7
  • 63
  • 76
0

I use the following solution. It relies on using the new keyword and overriding ESLint - but that override is scoped down to the file which contains the class.

What's important is that the class is private and what is exported is a plain function. Then when you use the function in the code, it's clear that it's not a class - it's just a regular function.

This way you can still use the class syntax w/o adding new code smell.

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

export function addSideEffects() {
  // eslint-disable-next-line no-new
  new SideEffects();
}
Maciej Krawczyk
  • 14,825
  • 5
  • 55
  • 67