jscheid commented 8 years ago

minifyify appears to be needed only at build time and thus shouldn't be included in the runtime (production) dependencies. I'm guessing the same goes for envify, unassertify and webworkify.

jfirebaugh commented 8 years ago

You would think so, right? But in fact browserify transforms need to be production dependencies, so that a dependent of mapbox-gl-js that wants to include mapbox-gl-js in the dependent's own bundle has the correct transforms available for mapbox-gl-js.

jscheid commented 8 years ago

minifyify is used by your build-min script, but why would I use that script in my project? I'm using webpack so I have a completely different build pipeline anyway, but even if I was using browserify, surely it should be my choice whether to apply minification to my assets and if so, which tool to use for it and at which stage to apply it.

I still don't see the need for including this in the production dependencies. I'm using mapbox-gl without minifyify, I am including it in our bundle, and it works fine. Am I missing something here?

lucaswoj commented 8 years ago

@jfirebaugh I think @jscheid is correct. The *ify packages are only used by npm run build-min and npm start. If a user is generating a bundle via require('mapbox-gl'), these dependencies are not needed.

jfirebaugh commented 8 years ago

Yes, you're correct about minifyify.

minifyify -- Used only by development scripts, can be a devDependency.
envify -- Seems to be unneeded (no uses of process.ENV). Can be dropped as a dependency.
unassertify -- Ideally should be listed in package.json browserify section so that downstream packages that bundle mapbox-gl-js do not contain debug assertions in production. Therefore it should remain a non-development dependency. However, I'm not sure if browserify provides a way to override the package.json browserify section, so that local debug builds do not use unassertify.

jscheid commented 8 years ago

Hi, thanks for taking another look at this.

It would be nice if browserify would not be (indirectly) pulled into production. I think that's what will happen if unassertify is a non-dev dependency (?)

Again, unassertify won't help people (like me) who use webpack. We'd have to use something different, webpack-unassert-loader or perhaps babel-plugin-unassert.

I can see that you have a desire to make things easier for browserify users, but even in that case, should it not be up to the downstream package's build script to make decisions such as whether or not to strip asserts, and which tool to use for it?

jfirebaugh commented 8 years ago

Neither browserify or unassertify will be pulled into the production bundle. They're not required by any runtime code. There is only some (small) overhead during npm install.

I definitely do want to make things easy for browserify issues -- the fact that one can use browserify without needing to review the bundling requirements of all dependencies (transitively!) is an important feature.

lucaswoj commented 8 years ago

envify -- Seems to be unneeded (no uses of process.ENV). Can be dropped as a dependency.

We use process.env in bench/index.js and debug/access-token.js (both used exclusively by npm start).

Next Steps

[x] move minifyify to devDependencies
[x] move envifyify to devDependencies
[x] add unassertify to package.json browserify transforms, disable using --ignore-transform=unassertify during debug builds

jscheid commented 8 years ago

Thanks @jfirebaugh , interesting link (especially the discussion on that webpack issue).

One thing I still don't quite understand though. From what I can tell, webworkify is an essential transformation without which the whole package breaks, whereas unassertify is purely an optimization. How come webworkify can be a devDependency and unassertify has to be a regular dependency?

lucaswoj commented 8 years ago

How come webworkify can be a devDependency and unassertify has to be a regular dependency?

It looks to me as though we ought to make webworkerify a regular dependency.

jscheid commented 8 years ago

Playing the devil's advocate... You have 300+ issues open and 1.6K closed, clearly your users are a vocal bunch, and not one has complained about it. Could that be an indicator that it's totally fine to have all the browserify stuff as devDependencies? I wonder what exactly is the symptom when it isn't.

lucaswoj commented 8 years ago

Most of our users use a pre-built version of GL JS so build issues can go unnoticed. Here is a failing test case:

$  git clone git@github.com:mapbox/mapbox-gl-js.git
...

$ cd mapbox-gl-js/

# this is how GL JS would be installed if it were a dependency
$ npm install --production
...

# this is nearly the same code path that would be triggered if a user called require('mapbox-gl') using browserify
$ npm run build-min

> mapbox-gl@0.20.1 build-min /Users/lucaswoj/Desktop/mapbox-gl-js
> browserify js/mapbox-gl.js --debug --transform unassertify --plugin [minifyify --map mapbox-gl.js.map --output dist/mapbox-gl.js.map] --standalone mapboxgl > dist/mapbox-gl.js

Error: Cannot find module 'webworkify' from '/Users/lucaswoj/Desktop/mapbox-gl-js/js/util/browser'
    at /Users/lucaswoj/Desktop/mapbox-gl-js/node_modules/browserify/node_modules/resolve/lib/async.js:46:17
    at process (/Users/lucaswoj/Desktop/mapbox-gl-js/node_modules/browserify/node_modules/resolve/lib/async.js:173:43)
    at ondir (/Users/lucaswoj/Desktop/mapbox-gl-js/node_modules/browserify/node_modules/resolve/lib/async.js:188:17)
    at load (/Users/lucaswoj/Desktop/mapbox-gl-js/node_modules/browserify/node_modules/resolve/lib/async.js:69:43)
    at onex (/Users/lucaswoj/Desktop/mapbox-gl-js/node_modules/browserify/node_modules/resolve/lib/async.js:92:31)
    at /Users/lucaswoj/Desktop/mapbox-gl-js/node_modules/browserify/node_modules/resolve/lib/async.js:22:47
    at FSReqWrap.oncomplete (fs.js:82:15)

npm ERR! Darwin 15.5.0
npm ERR! argv "/Users/lucaswoj/.nvm/versions/node/v4.4.5/bin/node" "/Users/lucaswoj/.nvm/versions/node/v4.4.5/bin/npm" "run" "build-min"
npm ERR! node v4.4.5
npm ERR! npm  v2.15.5
npm ERR! code ELIFECYCLE
npm ERR! mapbox-gl@0.20.1 build-min: `browserify js/mapbox-gl.js --debug --transform unassertify --plugin [minifyify --map mapbox-gl.js.map --output dist/mapbox-gl.js.map] --standalone mapboxgl > dist/mapbox-gl.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the mapbox-gl@0.20.1 build-min script 'browserify js/mapbox-gl.js --debug --transform unassertify --plugin [minifyify --map mapbox-gl.js.map --output dist/mapbox-gl.js.map] --standalone mapboxgl > dist/mapbox-gl.js'.
npm ERR! This is most likely a problem with the mapbox-gl package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     browserify js/mapbox-gl.js --debug --transform unassertify --plugin [minifyify --map mapbox-gl.js.map --output dist/mapbox-gl.js.map] --standalone mapboxgl > dist/mapbox-gl.js
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs mapbox-gl
npm ERR! Or if that isn't available, you can get their info via:
npm ERR! 
npm ERR!     npm owner ls mapbox-gl
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/lucaswoj/Desktop/mapbox-gl-js/npm-debug.log

jscheid commented 8 years ago

$ npm install --production # this is how GL JS would be installed if it were a dependency

I've tried the following with npm 2.15.6 and with 3.10.1 just now:

$ cd empty_directory
$ npm init
$ npm install --save mapbox-gl

In both cases I end up with webworkify in node_modules. (For npm 3.x it's on the top level, for 2.x it's nested below mapbox-gl).

Is this something that can happen when mapbox-gl is the dependency of a dependency?

lucaswoj commented 8 years ago

I encourage you to read the npm documentation and other resources to understand how npm's dependency system works.

jscheid commented 8 years ago

You must be kidding... both the npm docs and that SO answer are vague at best and misleading at worst.

From what I can tell in my tests, if A depends on B and B dev-depends on C, then when you run npm install in A, C will get installed. Only with three levels of indirection will the dev dependencies be skipped (A dep B dep C devDep D -- here D will not be installed).

This is different from what the docs and SO say:

If someone is [...] using your module in their program, then they probably don't want or need to download and build the external test or documentation framework that you use.

devDependencies are [...] not installed on npm install "$package" on any other directory, unless you give it the --dev option.

This also goes a long way to explain why nobody complains about webworkify. Since people will often use mapbox-gl directly in their app, rather than via a 3rd party package, they will still get webworkify installed even though it is a devDependency.

At any rate. You guys seem to know what you're doing and so I'll leave you to it. It's unfortunate that npm doesn't have a better way of modelling the dependencies, but it is what it is.

jfirebaugh commented 8 years ago

In the currently published version of mapbox-gl (0.20.1), webworkify is a regular dependency. It was (mistakenly) switched to a dev dependency in 4db866b57dfad747ace5b785638d30a7661adc48, which is not in any released version. So this explains both the behavior @jscheid is seeing, and the lack of any bug reports from browserify users.

jscheid commented 8 years ago

Oh... whoops. That explains a lot. I actually did have a look at package.json's history, because I was suspecting something along these lines, but obviously didn't look far enough. Sorry for the noise!

mapbox / mapbox-gl-js

minifyify should be a devDependency #2767

Next Steps