Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
givankin opened this issue Feb 18, 2014 · 23 comments
Closed

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

givankin opened this issue Feb 18, 2014 · 23 comments

Comments

@givankin
Copy link

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
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Member

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

  • taking package.json into account (issue title reflects it allright now)
  • loading from multiple root folders

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

@jamesshore
Copy link
Contributor

@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
Copy link
Author

@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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Member

@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
Copy link
Contributor

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
Copy link

+1 for resolving package.json.

@jazlalli
Copy link

jazlalli commented Mar 5, 2014

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
Copy link
Member

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

@jamesshore
Copy link
Contributor

@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
Copy link

jazlalli commented Mar 6, 2014

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
Copy link

zpratt commented Mar 18, 2014

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

@sebarmeli
Copy link

+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
Copy link
Author

givankin commented Aug 3, 2015

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
Copy link
Author

givankin commented Aug 4, 2015

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
Copy link

@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
Copy link
Contributor

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

@givankin
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants