meteor 1.3 - unable to resolve modules warnings

[rbabayoff]

@tmeasday, I'm getting warnings that I didn't get with rc.1:

=> Started proxy.

Unable to resolve some modules:

  "browser/escape-string-regexp" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/mocha.js
(web.browser)
  "browser/path" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/mocha.js
(web.browser)
  "growl" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/web.browser/mocha.js
(web.browser)
  "browser/tty" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/mocha.js
(web.browser)
  "browser/diff" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/mocha.js
(web.browser)
  "browser/fs" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/mocha.js
(web.browser)
  "jade" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/web.browser/mocha.js
(web.browser)
  "browser/events" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/mocha.js
(web.browser)
  "browser/debug" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/mocha.js
(web.browser)
  "browser/glob" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/mocha.js
(web.browser)
  "sass" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/node_modules/jade/lib/filters.js
(web.browser)
  "stylus" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/node_modules/jade/lib/filters.js
(web.browser)
  "less" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/node_modules/jade/lib/filters.js
(web.browser)
  "markdown" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/node_modules/jade/lib/filters.js
(web.browser)
  "discount" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/node_modules/jade/lib/filters.js
(web.browser)
  "markdown-js" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/node_modules/jade/lib/filters.js
(web.browser)
  "marked" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/node_modules/jade/lib/filters.js
(web.browser)
  "coffee-script" in
/home/vagrant/.meteor/packages/practicalmeteor_mocha/.2.1.1-rc.1.ih6myv++os+web.browser+web.cordova/npm/npm/node_modules/mocha/node_modules/jade/lib/filters.js
(web.browser)

Consider running: meteor npm install --save browser growl jade sass stylus less
markdown discount markdown-js marked coffee-script

I think it has something to do with adding mocha as an npm dependency, which we didn't before - we used the browserified one for both client and server.

@jsep can you please check if that's the case?

Steps to reproduce: just update an app with mocha tests from 1.3-rc.1 to 1.3-rc.13 and run meteor test --driver-package practicalmeteor:mocha

[tmeasday]

This appears to just be warnings that have been generated by analyzing some of the (redundant) files in the package. Is this possible @benjamn?

Does the package end up using jade for instance?

[rbabayoff]

@tmeasday and @benjamn it seems that you enabled warnings by default in rc.12:

https://github.com/meteor/meteor/compare/release/METEOR@1.3-rc.11...release/METEOR@1.3-rc.12?expand=1#diff-3396ab7aa5a3a2dbcfffa54934610c7bL854

Any plans to change that?

[benjamn]

No, many of these warnings are from files imported by the package. The problem is Browserify bundles like https://github.com/mochajs/mocha/blob/master/mocha.js have their own implementation of require, and our attempts to detect that special case in the past (and ignore those dependencies) met with worse problems.

I understand that the warnings are annoying, but turning them off is not an option if it comes that the cost of withholding useful information about unmet dependencies.

Perhaps you can import https://github.com/mochajs/mocha/blob/master/lib/mocha.js instead?

[rbabayoff]

@benjamn yes, we plan to, but we modified the mocha html reporter in the browserified version to be able to display 2 reportes in the same html page, so it will require a bit of work on our side - we want to create a pr into mocha itself with those changes, so meteor users can install whatever mocha version they want.

So, basically, no plan to restore this?

https://github.com/meteor/meteor/compare/release/METEOR@1.3-rc.11...release/METEOR@1.3-rc.12?expand=1#diff-3396ab7aa5a3a2dbcfffa54934610c7bL854

[benjamn]

Checking Console.isLevelEnabled(Console.LEVEL_WARN) was wrong because there's no way to change that warning level such that the check would ever pass, so unfortunately it's not as simple as un-reverting that change.

Even if we fixed the warning level logic to work properly, I'm not convinced these warnings should be hidden by default, because that's almost as bad as not having any warnings at all. And turning them on by default is probably not what you want for practicalmeteor:mocha. So what I'm hoping to settle on is a concrete proposal that balances these concerns.

Here's a good example of a time when the warnings were doing their job perfectly. I still think this is the more common case!

[benjamn]

This should be fixed if you do meteor update --release 1.3.1-rc.3, soon to become 1.3.1. In short, we're not warning about missing modules for Meteor packages anymore, and we're also not warning about require calls when require is not a free variable (e.g. in Browserify/Webpack bundles).

[trusktr]

@benjamn What's a "free variable"?

[jsep]

Hello guys, we just publish a new version of the package, the version is 2.4.5-rc.3. This version contains the latest mocha and should solve this problem as well with 1.3 meteor release. We now require mocha directly and we don't use the browserified version anymore.

[benjamn]

@trusktr within a given scope (such as a program, module, or function), a variable is "free" if it refers to a variable declared in some outer scope. In Node, require is usually a free variable, because you can refer to it without having to declare or import it. However, if you do something like

(function (require) {
  require("./foo");
})(...);

then there's no way to be sure require refers to the usual Node function, because the require variable is "bound" by the anonymous function parameter. The way I'm using these terms, "free" and "bound" are opposites, more or less.

When is require ever bound/unfree? The most common case is Browserify bundles, which wrap your modules in a closure that provides a Browserify-specific implementation of require. It would be a mistake to treat those require statements as ordinary dependencies, because presumably the Browserify bundle has already included everything you might require. Webpack does a better job of this by replacing require with __webpack_require__, so there's less confusion.