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

Allow development with npm link #33

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

artzhookov
Copy link
Contributor

@artzhookov artzhookov commented Aug 13, 2017

This PR fixes problem when npm link is used to link nci package to the project working directory (for example nci-quick-setup). Problem is described below. Also I made some refactoring in app/config.js and app/index.js

I have such folders structure:

nci
|--- nci                  (npm link)
|--- nci-classic-ui       (npm link)
|--- nci-quick-setup      (npm link nci, npm link nci-classic-ui)

When I run nci inside nci-quick-setup folder I get:

~/projects/nci/nci/app.js:8
	if (err) throw err;
	         ^

Error: Cannot find module 'leveldown'
    at Function.Module._resolveFilename (module.js:325:15)
    at Function.Module._load (module.js:276:25)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Group.self.projects.ProjectsCollection.db (/home/webster/projects/github/nci/nci/app/index.js:72:20)
    at Group.<anonymous> (/home/webster/projects/github/nci/nci/node_modules/twostep/lib/twoStep.js:171:15)
    at next (/home/webster/projects/github/nci/nci/node_modules/twostep/lib/twoStep.js:159:9)
    at Group.done (/home/webster/projects/github/nci/nci/node_modules/twostep/lib/twoStep.js:30:16)
    at Group._fillSlot (/home/webster/projects/github/nci/nci/node_modules/twostep/lib/twoStep.js:52:8)
    at /home/webster/projects/github/nci/nci/node_modules/twostep/lib/twoStep.js:77:9

This happens because nci tries to require leveldown, but leveldown is a dependency of nci-quick-setup (custom db backend).
Solving the problem is to do require call relative to cwd. I add import-cwd dependency and add method app.require that is used to require all external deps (db backend, plugins, proload.json).

@artzhookov
Copy link
Contributor Author

artzhookov commented Aug 13, 2017

Also I have another suggestion about development process:

  1. remove devDependencies
    "nci-projects-reloader": "1.1.3",
    "nci-rest-api-server": "1.0.4",
    "nci-static-server": "1.2.0",
    "nci-yaml-reader": "1.2.1",
  1. remove data folder
  2. remove nodemon

This package should be clear as much as possible because it is the "core" package.
For development process we should use separate package, that have nci in dependencies list (nci-quick-setup is a good candidate for this role). In this package nci and other nci-... packages could be linked with npm link (see my first comment).

@okv
Copy link
Member

okv commented Aug 16, 2017

I need to think about this suggestions carefully =)
I'll try to do that on next weekends.

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