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

Improve "No project root found" error message, simplify project root check to only look for package.json #395

Merged
merged 2 commits into from
Apr 18, 2017

Conversation

lencioni
Copy link
Collaborator

@lencioni lencioni commented Apr 12, 2017

This error message is too vague and can be confusing for folks. We can
make it better by providing some more context.

More info: Galooshi/atom-import-js#13


Stop checking for node_modules when finding project root

In #395 we discussed the logic
for determining that something is a project root. It seems that there
are situations where you might have a package.json file but no
node_modules directory (e.g. haven't run npm install yet, or there are
no dependencies). I think we can improve this by simplifying the check
to only look for package.json.

This error message is too vague and can be confusing for folks. We can
make it better by providing some more context.

More info: Galooshi/atom-import-js#13
@lencioni lencioni requested a review from trotzig April 12, 2017 15:45
Copy link
Collaborator

@trotzig trotzig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new message isn't strictly correct, and it makes me wonder if we could simplify to only look for a package.json file? The reason I didn't is because people might have local packages hidden deep in the tree structure, but I honestly don't know how vommon this is.

@lencioni
Copy link
Collaborator Author

What do you mean by "local packages hidden deep in the tree structure"? Can you give me an example of this?

@trotzig
Copy link
Collaborator

trotzig commented Apr 12, 2017

Something like

project
  - package.json
  - node_modules
    - ...
  - src
    - foo
      - package.json
      - foo-main.js
    - bar

The foo module here, specifying its main file as foo-main.js.

Never seen it in the wild, but it's valid, right?

@lencioni
Copy link
Collaborator Author

Ah I see. A sub-package that has no dependencies but has other package behavior. Yeah I could see that happening in monorepos. This could even happen if you just haven't run npm install in that directory yet. In any case, I think the existence of a package.json indicates the project root.

@trotzig
Copy link
Collaborator

trotzig commented Apr 13, 2017

Will you make that change? (Check for package.json only).

The error message in this PR is still better than what is there currently, so I'm fine merging.

In #395 we discussed the logic
for determining that something is a project root. It seems that there
are situations where you might have a package.json file but no
node_modules directory (e.g. haven't run `npm install` yet, or there are
no dependencies). I think we can improve this by simplifying the check
to only look for package.json.
@lencioni lencioni changed the title Improve "No project root found" error message Improve "No project root found" error message, simplify project root check to only look for package.json Apr 14, 2017
@lencioni
Copy link
Collaborator Author

@trotzig PR updated

@trotzig trotzig merged commit b752a53 into master Apr 18, 2017
@lencioni lencioni deleted the root-error branch April 18, 2017 17:25
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 this pull request may close these issues.

2 participants