Skip to content
This repository has been archived by the owner on Mar 3, 2020. It is now read-only.

feat(Ignitor): allow to set modulesRoot #11

Merged
merged 1 commit into from
Aug 23, 2018
Merged

feat(Ignitor): allow to set modulesRoot #11

merged 1 commit into from
Aug 23, 2018

Conversation

targos
Copy link
Member

@targos targos commented Aug 22, 2018

Proposed changes

Sometimes, appRoot is not necessarily the directory which contains
node_modules (for example when the AdonisJs app is in a subdirectory
of a bigger application.
ignitor.modulesRoot(location) allows to specify a different directory to
look for dependencies such as @adonisjs/ace.

Types of changes

What types of changes does your code introduce?

Put an x in the boxes that apply

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING doc
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation (if appropriate)

Sometimes, appRoot is not necessarily the directory which contains
node_modules (for example when the AdonisJs app is in a subdirectory
of a bigger application.
`ignitor.modulesRoot(location)` allows to specify a different directory to
look for dependencies such as `@adonisjs/ace`.
@targos
Copy link
Member Author

targos commented Aug 22, 2018

Documentation PR: adonisjs/legacy-docs#290

@coveralls
Copy link

Coverage Status

Coverage increased (+0.6%) to 93.827% when pulling 7e61fae on targos:modules-root into 8d4417b on adonisjs:develop.

Copy link
Member

@thetutlage thetutlage left a comment

Choose a reason for hiding this comment

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

Do you think, we should also change this line?

this._packageFile = require(path.join(this._appRoot, 'package.json'))

If your adonis code is not a separate app, I believe it will not have a package.json file too?

@targos
Copy link
Member Author

targos commented Aug 23, 2018

@thetutlage I'm hesitant about this one. The package.json is used to put the "autoload" field for the app. It seemed weird to me to have this configuration element outside of appRoot.

For example, in the app I am developing, I created a second package.json just for this:

{
  "private": true,
  "name": "api",
  "autoload": {
    "App": "./app"
  }
}

This is not ideal, but the other way of doing it would mean that one cannot have two Adonis apps with a different autoload config. Perhaps that's a limitation that just needs to be accepted as such...

@thetutlage
Copy link
Member

thetutlage commented Aug 23, 2018

but the other way of doing it would mean that one cannot have two Adonis apps

Having 2 Adonis apps sharing the same underlying dependencies will not help much either, since they both will be using the same IoC container.

But I do get your point and it's fine to keep that line as it is.

@thetutlage thetutlage self-assigned this Aug 23, 2018
@thetutlage thetutlage merged commit e1dda89 into adonisjs:develop Aug 23, 2018
thetutlage pushed a commit to adonisjs/legacy-docs that referenced this pull request Aug 23, 2018
@targos targos deleted the modules-root branch August 23, 2018 11:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants