search

jsreport / jsreport-core

The minimalist jsreport rendering core
GNU Lesser General Public License v3.0
86 stars 24 forks source link

Extensions auto discovery not working in some cases #2

Closed bjrmatos closed 8 years ago

bjrmatos commented 8 years ago

i was testing lastest jsreport (0.10) and found some cases where auto-discovery is not working..

1.- not working with jsreport installation instructions, so i have created a directory named jsreport-playground, then npm install jsreport --save , node node_modules/jsreport --init, node server.js, results -> Found 0 extensions (i haved added some console.log to see what is happening)

1

the bug is caused by this line, and can be solved by changing the condition to:

// to ignore cycles in any extension that have a dependency in jsreport or jsreport-core 
if (S(rootPath).contains('node_modules') && (S(file).endsWith('node_modules' + path.sep + 'jsreport') || S(file).endsWith('node_modules' + path.sep + 'jsreport-core'))) {

the solution is tested in npm 2 not sure if it will work in npm 3

2.- in the same app (jsreport-playground), currently if i need to install an additional extension to the app, jsreport doesn't recognize the extension listed in my app, only recognize extensions inside its node_modules (jsreport-playground/node_modules/jsreport/node_modules).. is the right behavior?

i think jsreport should recognize the extensions inside the application dependencies, if i'm not wrong jsreport was doing that in previous versions, must be a bug introduced by the split of jsreport, jsreport-core..

i think we should test very carefully if the auto-discovery feature (with jsreport-core and jsreport) works in npm 2 and 3 (since npm3 change the way to install node modules) , i only have tested in npm 2..

pofider commented 8 years ago

Ouch, I will check it on npm 2 and probably release hotfix 0.10.1 for this in the evening. Thank you.

pofider commented 8 years ago
  1. You were right. That condition was filtering out extensions. I have reimplemented the autodiscovery now in the way it breadth first search for jsreport.config.json. This means it will apply the extensions at the higher position in the node_modules over the extensions referenced somewhere deep. It works both on npm 3 and 2
  2. Yes this is bug for npm 2. I have fixed this now.

I am preparing hotfix 0.10.1 with required changes. It should be live in a hour.

bjrmatos commented 8 years ago

excellent! i'll continue testing later to see if there are other little bugs

pofider commented 8 years ago

It is in npm now. Works for me on both npm 2 and 3. You can reopen if it won't work for you.