-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Node module not recognized #46
Comments
As I noticed in #48 and #49 flow is using some own implementation following requires. It does not find modules from In an attempt to workaround #48 I removed the After that |
The thing is this a standard module in the node_modules folder in the same directory I ran flow init, not sure why it cannot find it. One I noticed is that the module doesn't have an index.js but instead a dist folder |
Our algorithm for finding node modules basically follows the spec at: http://nodejs.org/api/modules.html (We don't do anything special for |
@avikchaudhuri thanks for getting back to me, the react-router package json has the following
Notice the |
Similar problem with hapi
In their package.json file they have:
|
Changing @josebalius maybe you can try that with react-router too? |
@josebalius changing package.json to have the main include .js fixes this too for react-router. |
If this is resolved, can we close the issue? |
I have the same problem for the dat-gui package. In the package.json is no main attribute and there is an index.js file which should be used by default. |
The missing "main" attribute in the package.json is the problem. If I add it, then it does work. |
Also it seems if something is in |
@nnarhinen This is not entirely right. flow searches for a package.json, if there is one it only looks for the main attribute. If that is missing the resolving stops. If there is no package.json it looks for an index.js file, if that is missing the resolving fails. |
@KnisterPeter ok, well that's not how node works in this case. nodejs resolves |
@nnarhinen You are right with that. I just wanted to explain the flow implementation. |
@tcoopman yep that fixed the error, but flow should be able to handle this right? |
@avikchaudhuri @josebalius I believe that flow should indeed be able to handle this. Maybe someone can add this based on #78 |
@tcoopman @avikchaudhuri @josebalius I could look into this later this day and extend my pull request to be more node-resolving algorithm compliant. |
@avikchaudhuri @tcoopman @josebalius The pull request is extended with cases adding '.js' to the required path if no other resolution is found. |
@KnisterPeter thanks! keen to try it. I just upgraded to the new version, and it seems that @tcoopman's solution doesn't work anymore :( |
Which exactly do you mean? There are the node_tests which should contain all possible solutions. Which one is failing? Or which one is missing? |
@KnisterPeter your PR hasn't been merged yet, so I am pretty sure it's not your solution, but whatever code they push for the 0.1.1 release doesn't pick up any of my modules :( |
Seeing this
Which comes from:
Each of those modules have a |
cc @lordhoto regression in 0.1.1? |
At least with my patch applied all the node_tests run well. I'll executed them with 'flow check --all' |
@josebalius I've tested my workaround in 0.1.1 for react-router and it works for me. React is also correctly imported. Are you sure you don't have anything else that's changed? |
@tcoopman Yeah I am pretty sure, what's the best way to reset flow completely just in case? |
All, my mistake i added the node_modules folder to the ignore property in |
Any updates on these resolving problems? For me they are a show-stopper if I can't use flow on a browserify-based application. |
I've updated my patch yesterday morning. I think its finished now. |
@KnisterPeter Please give me a quick brief how I can test your patch and I'll let you know how it behaves on my existing project |
@nnarhinen If you don't rely on any other latest commits you could clone my fork (the patch is in the master branch), run make to compile flow and all should be ready to run. Otherwise you just need to clone the current flow master and add my pull-request on top and then compile flow. |
Ok @KnisterPeter I installed your fork. Here's the result:
The related requires are: var Bookshelf = require('backend/bookshelf');
var foos = require('backend/foo/foos'); // the path is a directory
var openssl = require('backend/foo/openssl'); Output of
|
@nnarhinen Can you please output 'ls -l node_modules/backend/' (Note the slash at the end which lists the content of the node_modules/backend folder) |
Relevant parts:
(no package.json nor index.js) ls -l node_modules/backend/foo/
ls -l node_modules/backend/foo/foos/
|
@nnarhinen The require('backend/bookshelf') you do is exactly like the 'node_tests/path_node_modules' unit test which does run successful on my setup. Can you please verify if it works for you or if it fails? Use 'flow check --all' to run the test case. |
FIX #46: Fallback to index.js if no main attribute in package.json
@KnisterPeter it's the symlinks that make it fail. I did
and it fails with
|
Anything on this issue ? |
Hi,
I have react-router installed as an npm module, and it's in the node_modules folder, but everything i run flow, i get the same error
I tried adding the node_modules folder to
.flowconfig
but no luck, any ideas? Thenode_modules
folder is at the same level as.flowconfig
The text was updated successfully, but these errors were encountered: