Cvmcosta / ltijs

Turn your application into a fully integratable LTI 1.3 tool provider.
https://cvmcosta.github.io/ltijs/
Apache License 2.0
300 stars 67 forks source link

Ability to provide an express app instead of ltijs making it for you #125

Closed danny-does-stuff closed 3 years ago

danny-does-stuff commented 3 years ago

First off I think this library is awesome and the work you've done is very impressive!

Is your feature request related to a problem? Please describe. I have an existing api with lots of endpoints, custom cors setup, logging, bodyparser, etc. I would like to be able to add ltijs to my application without having to change my entire express setup while hoping that my existing routes will still work and that the new lti routes will also work. Essentially the idea of the feature request is that a completely custom express server can coexist peacefully with the express server created by ltijs.

Describe the solution you'd like I would love to be able to pass in my existing express app when calling lti.setup(). This would mean lti would not need to setup an express app for me at all. I understand this would come with its own complications because of the custom cors setup in lti, the body parser, etc. Basically the dev (me) would have to make sure the express app had some specific configuration so it would work properly with the lti spec, making this a bit more difficult.

Describe alternatives you've considered

  1. Separate http server I think it would also be possible to run ltijs on a separate port and just have two express servers running. This way they don't interfere at all, and I just need to manage routing properly, although this may be more difficult to set up the routing into the nodejs application (i.e. something that routes myapp.com/lti/xxx to myapp.com/xxx:3001)

  2. More robust serverAddon option Another possibility would be to make the serverAddon option more robust. Currently you can create new middleware for your app, but this is all added right at the end of the setup function here https://github.com/Cvmcosta/ltijs/blob/02d12042de84ac9bb1a80db37eefe2559a5a1643/src/Utils/Server.js#L101. this happens after all other middleware has been setup, so there's no control before that. It could be nice to allow injecting middleware throughout the setup of the express app so the dev (me) could add middleware anywhere they want in the process. The caller would then use something like this

    lti.setup({...}, {...}, {
    serverAddon: {
      // These options are only a few ideas, nothing super well thought out!
      preMiddleware: (app) => {
         // Runs immediately after app is made. Let's you add middleware before any of the middleware that lti adds.
         // For example, you could call `app.use(routes)` right here instead so your own routes get called before lti middleware
      },
      postCors: (app) => {
         // Runs immediately after CORS middleware is added. Let's you add middleware that requires the cors functionality added by lti
         // not sure if this would even be useful...
      },
      postBodyParser: (app) => {
         // Runs immediately after bodyparser middleware is added. Let's you add more body parsing stuff so that the app can support more data types, such as images
      },
    })

Another option here would just be adding more options for configuring the different middlewares. Then the dev could configure any of the middlewares that lti adds. An example could look like this

lti.setup({...}, {...}, {
  config: {
    cors: {
      // config to be passed to the cors middleware
    },
    bodyParser: {
      // configure which types of bodyparsing you want
      text: true,
      json: true,
    },
    helmet: false, // turn off helmet for some reason, not sure why someone would want to do this
  }
})

In summary, alternative number 2 is giving more control of how the express app works to the developer. A problem I see with this alternative is that it would be much easier for the dev to mess up the setup and break the lti integration in the process.

  1. Separate node app that only handles LTI This alternative is just to make a completely new node process that is specifically for all LTI related things. Then we could just include ltijs as a dependency and setup a very simple service that handles LTI stuff. A concern I have with this approach is that this lti app and the existing node app where my other endpoints exist would probably need to communicate a lot, which would be more difficult if they are in separate services, whereas if they were in the same node app they would simply share all of their code.

  2. Use ltijs app as middleware itself This alternative may be the cleanest as long as it works as expected. With express I believe it's possible to use an express app as a router, and so doing something like this might work

lti.setup()

const app = express()

// Add lti's express app to the route `/lti`. Ideally this would keep all of lti's middlewares contained so they don't leak to my express app
app.use('/lti', lti.app)

// my custom middleware
app.use(bodyParser.json())
app.use(cors())
// ... etc, etc

// my custom routes
app.use('/api', require('./myRoutes.js')

// my custom error handler
app.use(err => {})

This may be the most favorable option, as long as express separates the apps all the way. If any of the middlewares (cors, bodyparser, logging, sessions, etc) bleed into each other at all then this would be a bit janky. This will probably be the option I attempt first as it would not require changes to ltijs.

Additional context Hopefully there is a good path forward for this feature that will benefit others as well! Thanks again for your hard work on this project, I know OSS is not easy!

Cvmcosta commented 3 years ago

Hello! If i understand correctly your request, you want to use Ltijs with another Express app. Alternative 4 is already possible using the serverless options described here in the documentation. Please let me know if that's what you are looking for.

danny-does-stuff commented 3 years ago

Wow huge facepalm 🤦‍♂️ . When the docs literally say "Deploying ltijs as part of another server" 😆 . Sorry to make you read through this novel! I'm glad at least one of my ideas was good enough to have been thought of before!

Cvmcosta commented 3 years ago

No problem hahaha I am working on getting a better documentation website so i hope it gets easier to find stuff :smile: .