afterRequirejsBuild.js is importing from node_modules (eek!)

[pixelzoom]

From Gruntfile.js:

// TODO: chipper#101 eek, this is scary! we are importing from the repository dir. ideally we should just have uglify-js installed once in chipper?
var uglify = require( '../../' + pkg.name + '/node_modules/uglify-js' );

[jonathanolson]

Agreed as scary, I remember briefly not figuring out a way to share chipper's node_modules. It would be nice to only require "npm install" in chipper, instead of everywhere.

[pixelzoom]

Is node_modules the only place where we can get uglify-js? Why can't we do require( 'uglify-js' ), like we're doing for assert and fs?

[jonathanolson]

Is node_modules the only place where we can get uglify-js? Why can't we do require( 'uglify-js' ), like we're doing for assert and fs?

uglify-js isn't a built-in node.js API like fs or assert, so it needs to be installed somewhere (npm install)

[pixelzoom]

Replacing this:

var uglify = require( '../../../' + pkg.name + '/node_modules/uglify-js' );

with this:

var uglify = require( 'uglify-js' );

seems to work just fine. What am I missing?...

[jonathanolson]

Not sure, let me check on my end.

[jonathanolson]

If we replace it with require( 'uglify-js' ), I have to have npm-installed both chipper and the sim. Without chipper node_modules:

$ grunt
Loading "Gruntfile.js" tasks...ERROR
>> Error: Cannot find module 'uglify-js'
Warning: Task "default" not found. Use --force to continue.

Aborted due to warnings.

without the sim node_modules:

$ grunt

path.js:116
        throw new TypeError('Arguments to path.resolve must be strings');
              ^
TypeError: Arguments to path.resolve must be strings
    at Object.exports.resolve (path.js:116:15)
    at module.exports (c:\Users\jon\AppData\Roaming\npm\node_modules\grunt-cli\node_modules\findup-sync\lib\findup-sync.js:26:26)
    at ChildProcess.<anonymous> (c:\Users\jon\AppData\Roaming\npm\node_modules\grunt-cli\bin\grunt:35:17)
    at ChildProcess.EventEmitter.emit (events.js:98:17)
    at Process.ChildProcess._handle.onexit (child_process.js:807:12)

[jonathanolson]

And our development overview just mentions npm install on the sim only.

[pixelzoom]

Could we put uglify-js in sherpa, and load it from there like we're doing for lodash?

[pixelzoom]

Btw... I'd also be fine with requiring npm-install for both chipper and sim.

[pixelzoom]

Note that jshint is being loaded like this, and doesn't require npm-install for chipper:

require( './jshintOptions' )

Why can't we do the same for uglify-js?

[pixelzoom]

Ah, sorry, that's loading jshintOptions.js. Never mind...

[jonathanolson]

Could we put uglify-js in sherpa, and load it from there like we're doing for lodash?

If you want to look into putting an npm package into sherpa and getting the require() to work out nicely, it's up to you. If we do put npm dependencies in sherpa, I'd recommend doing all of the following so we can run builds without an npm install of any kind:

    "grunt": "~0.4.1",
    "grunt-requirejs": "~0.4.0",
    "grunt-contrib-jshint": "~0.6.4",
    "uglify-js": "~2.4.0",
    "requirejs": "~2.1.8"

I don't anticipate that being clean in any way, since they all have sub-dependencies.

[jonathanolson]

Btw... I'd also be fine with requiring npm-install for both chipper and sim.

Or would it make sense to just have npm-install for chipper?

[jonathanolson]

Also on that note, it would solve a lot of my disk-space woes if we only had to npm-install for chipper. node_modules takes a lot of disk space if you have one for every simulation.

[samreid]

Or would it make sense to just have npm-install for chipper?

That sounds very desirable if it is technically feasible. So many node_modules directories I keep having to add to "exclude" lists at the moment :(

[pixelzoom]

+1 for investigating "chipper only" npm-install.

[pixelzoom]

This bit of code was moved from Gruntfile.js to afterRequirejsBuild.js. Updated issue title to reflect this.

[samreid]

I have to have npm-installed both chipper and the sim.

I skimmed through all of the sim package.json files and it looks like devDependencies contains "uglify-js": "~2.4.0" in each of them. So can we use var uglify = require( 'uglify-js' ); now?

[samreid]

By the way, the line will read:

var uglify = require( 'uglify-js' ); // eslint-disable-line require-statement-match

to avoid being reported as a name mismatch for eslint. @jonathanolson can you comment?

[jonathanolson]

to avoid being reported as a name mismatch for eslint.

Sounds like it's needed, can't think of a better way.

[samreid]

Sorry to be confusing, I was asking if we can switch from

var uglify = require( '../../' + pkg.name + '/node_modules/uglify-js' );

to

var uglify = require( 'uglify-js' );

now?

[jonathanolson]

Sorry!

I'm fine with chipper controlling the versions of those libraries (since it's the one that uses them, not the sim repo). Sounds fine with me, and it should be removed from the package.json files in the sim repos.

[samreid]

In this Feb 13 comment you reported a build error when using require('uglify-js') if the sim package.json doesn't contain a reference to uglify: https://github.com/phetsims/chipper/issues/101#issuecomment-74338310

Is that still a problem? If not, why not?

[jonathanolson]

Is that still a problem? If not, why not?

No idea, probably still a problem (sorry, thought you were asking if I was fine with that approach).

If the chipper and sim package.json request different versions, how do we handle that?

[samreid]

I don't understand why chipper and $sim have their own node_modules. When is a package such as uglify used from one and not the other? Or we can just assume that they both have equal versions or are otherwise compatible?

[jonathanolson]

I don't understand why chipper and $sim have their own node_modules.

As a hold-over due to our non-standard way of running grunt? It's likely that some chipper node_modules are unused.

[samreid]

I'm not really sure what best for this issue (or even possible in general). Perhaps good for N=2 developers to collaborate on. I'll unassign until then.