alphagov / govuk-prototype-kit

Rapidly create HTML prototypes of GOV.UK services
https://prototype-kit.service.gov.uk
MIT License
303 stars 236 forks source link

Need to access `app` and other variables in `server.js` #1718

Closed joelanman closed 1 year ago

joelanman commented 1 year ago

from support:

In v13, users can no longer access server.js by design - a clearer separation between user code and prototype kit code is a good thing.

However some users need to access things that are inside server.js, for example app and nunjucksAppEnv:

const flash = require('connect-flash') // for flash messages
const markdown = require('nunjucks-markdown') // for nunjucks markdown tag
const { marked } = require('marked')

app.use(flash())

markdown.register(nunjucksAppEnv, marked)
joelanman commented 1 year ago

To note: if code doesnt require access to app it can often be achieved in app/routes.js, for example to add middleware:

router.use(function(req, res, next){
  req.locals.myVar = "test"
  next()
}
edwardhorsford commented 1 year ago

Some things we have modified in server.js:

All of these are things I think the kit could include by default though...

joelanman commented 1 year ago

From further investigation with @adamsilver, it's possible to use Flash via routes:

router.use(flash())

and you can add Markdown as a filter

We've also raised a new ticket to investigate adding middleware in plugins

edwardhorsford commented 1 year ago

I mentioned on Slack, but I think this and many of the other needs could be solved by having a configuration approach similar to Eleventy - In short you have an optional eleventy.js file that exports a function. That function receives the eleventyConfig - so you can read all the current settings, get access to app, env, change settings etc. You then return an object with anything else you want to set. It could expose methods for setting various things in the kit.

It would also be backwards compatible - the kit could work without it, or accept the input it gives. It also gives a way for more features to be added over time - the config could expose new methods for doing things.

joelanman commented 1 year ago

we're going to close this issue in favour of people opening specific issues about the features they need. Same for the idea of a 'configuration approach' - it would be more useful to know the needs behind it in order to know what the best solution is

edwardhorsford commented 1 year ago

It's great if common needs can be met by the kit (and we can have issues to cover those), but as a more advanced user of the kit I've got a need to be able to adapt the prototype as needed, without needing to ask here first. This was possible in the previous prototype as you had full control of it - but no longer.

Philosophically the team may have decided that users of the kit should no longer have access to the core internals - so be it - but I still would treat it as a need.

joelanman commented 1 year ago

As a team we need to know use cases so we can support them in the best way for all our users. From research we know that the previous approach of allowing anyone to change anything results in serious issues:

It may be that the solution to certain use cases is very flexible, for example via the Plugins framework, or even a config file as suggested, but we need to know the use cases, and design and document solutions that work for everyone without re-introducing issues.

Please do raise any use cases so we can research and prioritise.

edwardhorsford commented 1 year ago

I'm very supportive of the team knowing the use cases - I gave four examples above.

I don't think the team wanting to know about use cases is incompatible with a user need to have access to app without needing to ask for permission first.

You might decide not to meet this need - because of the reasons you've stated - but I wouldn't disregard that this isn't a need some users have.

joelanman commented 1 year ago

Related specific issues:

Feel free to raise more

We don't currently have an issue for Flash though it seems easy to use it via routes as above

lfdebrux commented 1 year ago

If it helps, you can add the following to app/routes.js to get the Nunjucks environment and add the Markdown extension

// Markdown support
const markdown = require('nunjucks-markdown')
const marked = require('marked')

// One time setup
let setupDone = false
router.use((req, res, next) => {
  if (setupDone) return next()

  console.log('route.js: doing one time setup')
  markdown.register(req.app.get('nunjucksEnv'), marked.parse)
  setupDone = true
  next()
})

Oh, and just to add to the hackiness, because the kit is shrinkwrapped, when you install nunjucks-markdown you will get another copy of nunjucks installed into your node modules folder. This wouldn't be an issue, except for the fact that nunjucks checks whether a string is safe or needs to be escaped by doing a instanceof check... which will always be false when the markdown renderer is using a different instantiation of the nunjucks module. Luckily it's easy to work around, you just need to tell npm to use the copy of nunjucks that comes with the kit in the prototype, by running npm install nunjucks file:node_modules/govuk-prototype-kit/node_modules/nunjucks 😈

It's not pretty, but it works!

lfdebrux commented 1 year ago

I was finding some weird issues with npm when using the above solution, so as an alternative I've created a fork of nunjucks-markdown that is designed to work with the kit: https://github.com/lfdebrux/govuk-prototype-kit-nunjucks-markdown-plugin