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

manage file not found errors #4

Closed
mef opened this Issue Jul 12, 2017 · 5 comments

Comments

2 participants
@mef

mef commented Jul 12, 2017

In some repositories, I deliberately exclude files containing some sensitive information (e.g. API keys stored in a params.json file).

node-dependencies-view then fails at generating the svg, producing the following error message:

{ Error: Cannot find module '../params.json'
    at Function.Module._resolveFilename (module.js:469:15)
    at resolveFileName (/tmp/node-dependencies-view/node_modules/resolve-from/index.js:17:39)
    at resolveFrom (/tmp/node-dependencies-view/node_modules/resolve-from/index.js:31:9)
    at module.exports (/tmp/node-dependencies-view/node_modules/resolve-from/index.js:34:41)
    at inspect (/tmp/node-dependencies-view/dependency-tree.js:32:22)
    at module.exports (/tmp/node-dependencies-view/dependency-tree.js:14:24)
    at download.then.then (/tmp/node-dependencies-view/graph.js:16:16) code: 'MODULE_NOT_FOUND' }
TypeError: Cannot read property 'split' of undefined
    at graph.then.dot (/tmp/node-dependencies-view/server.js:27:24)

Two possible solutions are:

  • skip the file not found (and possibly warn about incomplete results in the produced svg)
  • catch the error, and concatenate baseand req, instead on relying on function resolve.

This second solution worked well for me:

try {
	var resolved = resolve(path.dirname(file), req)
} 
catch(e) {
	var resolved = base + req
}
let rel = '.' + resolved.slice(base.length)

Would you consider a pull request for this?

@fiatjaf

This comment has been minimized.

Show comment
Hide comment
@fiatjaf

fiatjaf Jul 12, 2017

Owner

Wait, what? So you're telling me that resolve-from actually checks a file for its existence? I thought it would just perform some string manipulation with pathnames. That would work, right? I think we can just remove resolve-from entirely and replace it with https://nodejs.org/api/path.html#path_path_resolve_paths

If you want to try that out and send a PR I will gladly accept it. Otherwise your solution will work too, but I really prefer to NOT check for file existence, that's useless filesystem bloat.

If you don't want to send a PR, show me at least a repo of yours with a missing file like that so I can use it for testing (as you can see, there are no unit tests in this project -- yes, I'm a horrible programmer).

Owner

fiatjaf commented Jul 12, 2017

Wait, what? So you're telling me that resolve-from actually checks a file for its existence? I thought it would just perform some string manipulation with pathnames. That would work, right? I think we can just remove resolve-from entirely and replace it with https://nodejs.org/api/path.html#path_path_resolve_paths

If you want to try that out and send a PR I will gladly accept it. Otherwise your solution will work too, but I really prefer to NOT check for file existence, that's useless filesystem bloat.

If you don't want to send a PR, show me at least a repo of yours with a missing file like that so I can use it for testing (as you can see, there are no unit tests in this project -- yes, I'm a horrible programmer).

mef added a commit to Mango-information-systems/node-dependencies-view that referenced this issue Jul 12, 2017

replaced resolve-from with resolve fiatjaf/node-dependencies-view#4
Solves the error preventing svg to be returned whenever a module is not found
Problem: file extensions are not appended anymore to the required modules. This causes the svg to reference the same module twice, e.g.  and , considered different, in separate positions in the graph :/
@mef

This comment has been minimized.

Show comment
Hide comment
@mef

mef Jul 12, 2017

Thanks.

I just tried to replace resolve-from by path.resolve, however it does not produce the expected result.

Required modules do not get their file extensions appended, and are considered as different entities from the actual files (resulting in the same module being shown twice in two different places in the graph) 🤔 .

Do you see a solution for this? Perhaps that another approach is to be considered...

mef commented Jul 12, 2017

Thanks.

I just tried to replace resolve-from by path.resolve, however it does not produce the expected result.

Required modules do not get their file extensions appended, and are considered as different entities from the actual files (resulting in the same module being shown twice in two different places in the graph) 🤔 .

Do you see a solution for this? Perhaps that another approach is to be considered...

@fiatjaf fiatjaf closed this in 5b1aa5c Jul 12, 2017

@fiatjaf

This comment has been minimized.

Show comment
Hide comment
@fiatjaf

fiatjaf Jul 12, 2017

Owner

I don't know why you replaced the , with a + 😛 but it is working now.

Owner

fiatjaf commented Jul 12, 2017

I don't know why you replaced the , with a + 😛 but it is working now.

@fiatjaf

This comment has been minimized.

Show comment
Hide comment
@fiatjaf

fiatjaf Jul 12, 2017

Owner

Forget about it. You were right. It is not. I've broken everything.

(That's why you should always have unit tests.)

Owner

fiatjaf commented Jul 12, 2017

Forget about it. You were right. It is not. I've broken everything.

(That's why you should always have unit tests.)

@fiatjaf fiatjaf reopened this Jul 12, 2017

@fiatjaf fiatjaf closed this in 9e3ccf8 Jul 12, 2017

@mef

This comment has been minimized.

Show comment
Hide comment
@mef

mef Jul 12, 2017

Thanks 👍

mef commented Jul 12, 2017

Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment