karma-runner / karma-commonjs

A Karma plugin. Test CommonJS modules.
MIT License
73 stars 30 forks source link

package.json is not used to resolve module path #16

Closed givankin closed 10 years ago

givankin commented 10 years ago

If one has a dependency that looks like this:

node_modules/a-library/
  package.json
  main-library-file.js

And in package.json:

...
main: "./main-library-file"
...

Then there is a

Error: Could not find module 'node_modules/a-library.js' from 'x.js'

From a brief look at source code I suppose that karma-commonjs doesn't parse package.jsons. Maybe it's fine, doing it in the browser would be strange, but this means (correct me if I'm wrong) that I have to use built JS to run tests. And that's what I'd like to avoid because build procedure can grow complex and slow and running it on every "Shift+F10" makes people sad. What if a special "map" is introduced in config file, so we can do

'a-library': 'node-modules/a-library/main-library-file.js'

or

'node-modules/a-library/main-library-file.js:a-library'

somewhere, in the spirit of browserify-shim and similar libraries? This would also make karma-commonjs feasible in cases where the dependencies are not in node_modules folder (e.g. if bower+debowerify is used).

pkozlowski-opensource commented 10 years ago

@abbakym you are right that the plugin doesn't read package.json at the moment. The idea of a map was discussed before and it kind of makes sense, it is just that none of maintainers had a immediate need for resolution from multiple folders. If you've got a use-case and fancy sending a PR I could help a bit as I might have (in a distant future...) a use case for resolution from multiple root folders.

jamesshore commented 10 years ago

I've updated the title.

I agree that this is something we should support. I'd like to find a way to do it without requiring users to manually duplicate the package.json "main" entry, although I'm not sure there's an easy way to do so. I think we'd have to read the entire node_modules tree at startup parse the package.json files within, then inject those mappings into the browser-side process.

pkozlowski-opensource commented 10 years ago

@jamesshore I think that @abbakym it in fact raising 2 issues here:

@abbakym if you need both perhaps it would be easier to have it as a separate issue.

jamesshore commented 10 years ago

@pkozlowski-opensource Hmm, you might be right. That's not how I read it.

@abbakym, if you want to change the node_modules directory to something else, you can use the modulesRoot: 'some_folder' config option described in the readme. If you want to have multiple module directories, please open a new issue.

givankin commented 10 years ago

@jamesshore, @pkozlowski-opensource, thanks for your feedback.

Regarding multiple root folders I am in doubt. I am a very recent convert from AMD and what I see in browserify (and, more broadly, in CommonJS front-end) world is that there is much more confusion than there is on a surface ("love browserify and stop worrying" hype). You have bower, components, globals, AMD--and then debowerify, decomponentify, deblobalify etc. etc. While this is the state of affairs which is unlikely to change dramatically in the near future, it would be nice to have a suggested workflow that could cover as much of this as possible. E.g. if I saw in karma-commonjs README: "keep all you dependencies in node_modules, and then we will take good care of them", and this would "just work", I would be happy. On the other hand, there may be cases where it won't work well. E.g. what if a library doesn't have a proper package.json? Then again users would want "mapping" -- and it may be more than just for multiple root folders. So maybe parsing package.json + mapping for extra cases?

givankin commented 10 years ago

BTW, besides not parsing package.json, karma-commonjs tries to load dependencies by their immediate names, e.g. root/lodash.js and not root/lodash/lodash.js. Altogether for me it fails to load nearly all the dependencies.

What I ended up doing is setting up a build task that copies all the .js dependencies from node_modules to a special folder (say tmp) and then adding this folder to karma.conf.js. Definitely it would be easier to do using maps ;-)

Maybe I am missing something. Can anyone suggest a happy workflow that works for you and allows both to "compile" the code quickly and to run unit tests on (uncompiled) modules? Thanks very much in advance.

jamesshore commented 10 years ago

BTW, besides not parsing package.json, karma-commonjs tries to load dependencies by their immediate names, e.g. root/lodash.js and not root/lodash/lodash.js. Altogether for me it fails to load nearly all the dependencies.

That's related but slightly different. I'll open an issue for it.

jamesshore commented 10 years ago

@pkozlowski-opensource, can you help me understand this issue better? Please correct any misunderstandings below:

  1. We have two separate processes that are involved with loading modules. First is the CommonJS Preprocessor that runs inside of the Karma node.js process. Second is the CommonJS Bridge that runs inside the browser.
  2. The Preprocessor can load files (because it's running in Node.js), but the Bridge can't (because it's running in the browser).
  3. The Preprocessor sets up a data structure in window.__cjs_module__ that allows the Bridge to look up modules by their path in the file system. By the time the Bridge runs, that data structure is fully populated. So the Bridge knows the paths of every file processed by the Preprocessor even though it doesn't have access to the file system.
  4. The Preprocessor also sets up window.__cjs_modules_root__ to point to the location of node_modules (or whatever), which allows the Bridge to look up modules without a prefix. This is a bit of a hack, because the specified behavior is to recursively scan up the tree looking for a node_modules directory.
  5. We do not support folders as modules at all. Because most (or all?) npm modules are folders, not files, that makes this plugin pretty unusable for people who want to require npm modules client-side.
  6. Supporting folders as modules is a challenge because you have to parse package.json and we can't read files in the Bridge. But other than the "main" property in package.json, window.__cjs_module__ has all the path information and modules we need (assuming the preprocessor is configured properly in karma.conf.js).

Is this correct so far?

jamesshore commented 10 years ago

Also,

  1. The Preprocessor doesn't know (and can't know) which files the Bridge will need, because require() calls are processed at runtime in the browser.
pkozlowski-opensource commented 10 years ago

@jamesshore your statements are exact according to my knowledge.

Regarding (5) it is partly-true only as people can still require an exact file inside a module, ex.: require('lodash/lodash'). Also, many npm modules will have index.js entry point which we could easily support without messing with package.json

jamesshore commented 10 years ago

Would it be possible for us to add support for package.json to the Preprocessor? In other words, karma.conf.js would look like this:

    preprocessors: {
      '**/*.js': ['commonjs'],
      '**/package.json': ['commonjs']
    }

Then the Preprocessor would parse any package.json files it finds, add the main property to a global data structure as we do with .js files, and then the Bridge could read it and take appropriate action. Could that work?

dcrockwell commented 10 years ago

+1 for resolving package.json.

jazlalli commented 10 years ago

I'm just running into this issue today, so a bit late to the party, but hopefully some of this is useful...

@pkozlowski-opensource regarding your last comment, requiring an npm module by using the full path doesn't work either, e.g.

I'm doing require('socket.io-client/dist/socket.io') and getting the error

Error: Could not find module 'C:/GitHub/basin/clients/js/node_modules/socket.io-client/dist/socket.io.js' 
from 'C:/GitHub/basin/clients/js/src/channel.js'
at C:/GitHub/basin/clients/js/node_modules/karma-commonjs/client/commonjs_bridge.js:21

All of those paths/files exist and are valid. The relevant section of karma.config.js is

// base path, that will be used to resolve files and exclude
basePath: '',

// frameworks to use
frameworks: ['jasmine', 'commonjs'],

// list of files / patterns to load in the browser
files: [
  'src/**/*.js',
  'tests/**/*.js'
],

preprocessors: {
  'src/**/*.js': ['commonjs'],
  'tests/**/*.js': ['commonjs']
},

Let me know if you want any further info. Cheers.

pkozlowski-opensource commented 10 years ago

@jazlalli could you put together a minimal project somewhere on GitHub demonstrating that full path resolution doesn't work?

jamesshore commented 10 years ago

@jazlalli It looks like you need to add socket.io to the preprocessors block, i.e.,

preprocessors: {
  'src/**/*.js': ['commonjs'],
  'tests/**/*.js': ['commonjs'],
  'node_modules/socket.io-client/dist/socket.io.js': ['commonjs']
},
jazlalli commented 10 years ago

hi folks, thanks for your replies

@jamesshore unfortunately, no change after adding socket.io to preprocessors, still throws the same error.

@pkozlowski-opensource I'll try and throw something together shortly. Will get back to you when I have something.

Cheers

zpratt commented 10 years ago

I have experienced this issue as well. +1 for a fix as I think there is serious value in implementing this.

sebarmeli commented 10 years ago

+1 for fixing this.

I could partially make it work for some packages hacking the config. E.g:

  files: [
    'client/*.js',
    'test/client/*.js',
    'node_modules/..../file.js
  ],

   preprocessors: [
    'client/*.js': ['commonjs'],
    'test/client/*.js': ['commonjs'],
    'node_modules/..../file.js: ['commonjs']
  ],

This will work for packages where the filename is the same as the package name. E.g.

require('x');

and in the package (inside node_modules) there is a file x.js

This is hacky and it won't work for all the packages.

Looking at the Karma project though, they are using karma-commonjs and it seems to work, so not sure what's going on.

givankin commented 9 years ago

Unfortunately can't get this to work in 0.0.13.

I get an

Uncaught Error: Could not find module 'lodash'.

The error happens in the loadAsDirectory function: it checks if lodash's package.json is in "existing files", and because it is not, tries to load index.js - after which everything fails. As "existing files" is effectively window.__cjs_module__, I assumed that I have to load the package.json explicitly.

But after I load it like this:

    files: [
      'node_modules/lodash/package.json',
      //...
    ],
    preprocessors: {
      'node_modules/lodash/package.json': ['commonjs'],
      //...
    },

It throws that window.__cjs_module__ is undefined. Indeed, if I look at preprocessed package.json, it looks like this:

window.__cjs_module__["/<...>/node_modules/lodash/package.json"] = {...}

Whereas other preprocessed JS files all look like this:

window.__cjs_modules_root__ = "<...>/node_modules";window.__cjs_module__ = window.__cjs_module__ || {};window.__cjs_module__["<...>/foo.js"] = function() { ... }

Have I missed something? How is this supposed to be configured?

Thanks a lot in advance for your help.

givankin commented 9 years ago

I finally got package.json loaded by placing it after some common-js-preprocessed JS files in the files array in karma.conf.js. It loads fine without an exception, as __cjs_module__ is non-empty when it loads.

However this line is now making trouble:

requiringPathEls.shift(); //cut of initial part coming from /

I suppose it is usable on Unix/Linux (though I can't clearly imagine why), but on Windows it strips the "C:" part. And later, when loadAsDirectory is matching the path to package.json to "existing files", it doesn't find one, because in "existing files" the path indeed does have the "C:" part.

DaveEmmerson commented 8 years ago

@givankin did you ever resolve this? It's a year on and I'm going round in circles hitting the same issues and also got as far as stepping through the code to see what was going on and came to the same conclusion about the / stripping.

jamesshore commented 8 years ago

@DaveEmmerson The Windows issue is a bug, which I fixed in #54. Just waiting for @pkozlowski-opensource to merge it.

givankin commented 8 years ago

@DaveEmmerson what I ended up doing is creating a (pretty complicated) set of grunt tasks that assemble all the dependencies for me and put into a special folder specifically for tests, and then I have

commonjsPreprocessor: {
  modulesRoot: 'src/dependencies/'
},

in my karma.conf.js file. This is cumbersome, but once configured it generally continues to work.