Rewrite bad-sim-text toFixed regex without lookbehinds

[zepumph]

Javascripts flavor of regex doesn't support lookbehinds, and it is causing an error for #728 (comment).

See https://stackoverflow.com/questions/4200157/javascript-regular-expression-exception-invalid-group

note from that article that Javascript does support lookaheads, so I will look into that.

Right now I will comment it out, and try to add things back in later once fixing the regex.

[pixelzoom]

The regex to identify usages of builtin toFixed is apparently what this issue is about.

When this issue is resolved, please remove the corresponding item from the code review checklist.

[zepumph]

Looking at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp, it looks like it is supported in node 8.10. Since this is just for eslint, and not in sim code, I wonder if I could convince everyone on the team to update to at least that version. Marking for dev meeting to see what public opinion is.

[zepumph]

To be clear. The regex I wrote in bad-sim-text has always worked for me, as I use Node 10, but it won't work if you have a lower version than Node 8.10. Would people be willing to update their node? Presumably this would mean servers too, so I'm unsure this is worth it.

[zepumph]

@jessegreenberg back in #728 (comment) you reported a comment where this lint rule didn't work for you. Privately I just asked you what your node version was, and you said 8.11. I think that would be a great test to see if it now works in lint. Before discussing with devs, would you please uncomment the lint rule in bad-sim-text and see if lint-everything will run for you now?

[jessegreenberg]

I uncommented the rule and it appears to be working well. It is reporting 16 files using native toFixed instead of DOT/Util.

[zepumph]

Thanks!

[pixelzoom]

@zepumph said:

... I wonder if I could convince everyone on the team to update to at least that version [8.10+]

I'm running node --version => v10.15.1

@jessegreenberg said:

I uncommented the rule and it appears to be working well.

Then does this still need to be labeled for developer meeting?

[zepumph]

Then does this still need to be labeled for developer meeting?

The reason this is marked for dev meeting (from #737 (comment)), is that I'm interested in making our eslint process dependent on Node 8.10. It sounds like most people are already running this, but I thought it worth a dev discussion. I see bayes is on 8.16, and phet-server is on 8.9, so it would likely need an update. If it isn't too much trouble, my preference would be to update Node versions to get this working. I think it would good and future proof to support regexes like this.

[zepumph]

phetsims/phet-info#59 could also be fixed very easily with this regex support.

[zepumph]

This is on hold until we make sure that eslint will run on all servers will run it successfully. On hold until https://github.com/phetsims/special-ops/issues/147 is done

[zepumph]

I will see about documenting the version dependency, perhaps as a lint rule.

[pixelzoom]

How about checking the node version as part of the grunt task that has the dependency? Pseudo code:

if ( node version < 8.10 ) {
  "node 8.10+ is required"
  exit
}

[zepumph]

https://github.com/phetsims/special-ops/issues/147#issuecomment-539284274 is complete, I think we can move forward over here.

[zepumph]

How about checking the node version as part of the grunt task that has the dependency? Pseudo code:

Implemented above. This is a very good idea, thanks @pixelzoom.

[zepumph]

From work in done in #791, this was quite easy to add to getBadTextTester. The issue is that when turning it on, there are now 48 lint errors. Likely many of them should be converted to use Util.toFixed, but I found that there there were some that I had questions about.

For example @jonathanolson, in dot/Matrix4.js, should we be using Util.js? Are there any usages in any of the below files that we don't want to use DOT/Util.toFixed?

• bending-light\js\intro\view\AngleNode.js
• density-buoyancy-common\js\density\view\DensityTableNode.js
• dot\js\Matrix3.js
• dot\js\Matrix4.js
• dot\js\Util.js
• fluid-pressure-and-flow\js\common\view\VelocitySensorNode.js
• fluid-pressure-and-flow\js\flow\model\FlowModel.js
• fluid-pressure-and-flow\js\flow\view\FluxMeterNode.js
• fluid-pressure-and-flow\js\watertower\model\WaterTowerModel.js
• joist\js\splash.js
• kite\js\kite.js
• optics-lab\js\optics-lab\view\ControlPanel.js
• optics-lab\js\optics-lab\view\SelectedPieceControlPanel.js
• scenery\js\display\drawables\ImageSVGDrawable.js
• scenery\js\display\SVGGradientStop.js
• scenery\js\util\Color.js
• states-of-matter\js\common\model\MoleculeForceAndMotionDataSet.js
• tambo\js\soundManager.js
• wave-interference\js\diffraction\view\DiffractionNumberControl.js

If not, I will likely turn this into a chip-away issue before turning this lint rule back on. Over to @jonathanolson to answer my question above.

[jonathanolson]

The dot/kite/scenery occurrences above should stay as they are (if they need eslint line disables, those should be added). I fixed up the density occurrence.

[zepumph]

There are now 55 usages.

[zepumph]

I found that the current version of eslint didn't parse regex negative lookbehinds. When I updated to ~7.22.0 the issue was fixed. I updated above.

[zepumph]

Actually, eslint doesn't seem to recognize negative lookbehinds. I'm going to make an issue in their repo.

UPDATE: converting: /(?<!Utils)\.toFixed\(/ to new RegExp( '(?<!Utils)\\.toFixed\\(' ) fixed it.

[zepumph]

Ok. I fixed all the usages (not in dot/kite/scenery) and commented in the lint rule. We can now enforce using Utils.toFixed and not gain more usages of Number.toFixed().

Closing.