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

Not working in IISNode after 1.4.0 #281

Closed
pofider opened this issue Apr 12, 2017 · 9 comments
Closed

Not working in IISNode after 1.4.0 #281

pofider opened this issue Apr 12, 2017 · 9 comments
Labels

Comments

@pofider
Copy link
Contributor

pofider commented Apr 12, 2017

The cli introduced using require.main inside the server.js.

if (require.main !== module) {
  // export jsreport instance to make it possible to use jsreport-cli
  module.exports = jsreport
} else {
  jsreport.init().then(function () {
    // running
  }).catch(function (e) {
    // error during startup
    console.error(e.stack)
    process.exit(1)
  })
}

Unfortunately the call require.main !== module is always true in the IISNode, so the application never starts.

https://forum.jsreport.net/topic/57/iisnode-error-500

@pofider pofider added the bug label Apr 12, 2017
@pofider
Copy link
Contributor Author

pofider commented Apr 12, 2017

Workaround is to use only the following in the server.js.

jsreport.init().then(function () {
    // running
  }).catch(function (e) {
    // error during startup
    console.error(e.stack)
    process.exit(1)
  })

@phobos04
Copy link

I can confirm the workaround works.
Looking forward to have it merged on a newer version.
Thank you.

@pofider
Copy link
Contributor Author

pofider commented Apr 12, 2017

The problem with iisnode is that it gives a unexpected value for require.main. Something like

Module {
  id: '.',
  exports: {},
  parent: null,
  filename: 'C:\\Program Files\\iisnode\\interceptor.js',

I don't want to make the server.js even more complicated than now by adding some kind of /iisnode/ condition.

  1. Wouldn't it be more safe to use environment variables to find out if the module was required from cli?
 if (process.env.JSREPORT_CLI) {
  module.exports = jsreport
} else {
  jsreport.init().then(function () {
  }).catch(function (e) {
    console.trace(e)
    process.exit(1)
  })
}
  1. Or even shadow this whole condition into the jsreport itself and make the server.js less confusing and simple.
 module.exports = jsreport.init().then(...).catch(...)

and in jsreport

reporter.init = function () {
   if (process.env.JSREPORT_CLI) {
      return Promise.resolve(this)
  } 
}

@bjrmatos What are your thoughts?

@bjrmatos
Copy link
Collaborator

ugh nice catch

i would prefer explicit export using the env JSREPORT_CLI, changing the behaviour of .init in this context doesn't makes sense to me and it feels too magic, explicitness always wins 😄

@pofider
Copy link
Contributor Author

pofider commented Apr 12, 2017

I was exactly expecting this answer :)

we can start with the first proposal, but I will still keep an eye if we won't find better solution in the end.
I don't like that confusing if. It is something nobody cares about. It is jsreport internal thing that the cli needs a uninitialized jsreport instance. Why to bother everyone with it? Hope we find an explicit and non disturbing way in the end. For now we stick with proposal 1.

@bjrmatos
Copy link
Collaborator

I don't like that confusing if. It is something nobody cares about. It is jsreport internal thing that the cli needs a uninitialized jsreport instance. Why to bother everyone with it? Hope we find an explicit and non disturbing way in the end. For now we stick with proposal 1.

yeah, i don't like either but i think it does the job very well, at least if you look at the file now you will think "ah ok this part is to make the jsreport cli work" (we even have a better hint now that we are going to use JSREPORT_CLI ), very explicit, but sure maybe is there a better way. let's see 😄

@bjrmatos
Copy link
Collaborator

bjrmatos commented Apr 12, 2017

changes done, the change will be part of the next jsreport release

@pofider
Copy link
Contributor Author

pofider commented Apr 13, 2017

Yes exactly, I believe this is now better with environment var because it gives hint why it is there.

@bjrmatos
Copy link
Collaborator

fixed in 1.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants