-
Notifications
You must be signed in to change notification settings - Fork 237
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
Add backwards compatibility - support for prototypes made in Version 6 of the kit #553
Conversation
5896412
to
e663767
Compare
d15543a
to
5a63a5c
Compare
44cfbb1
to
c3da12f
Compare
|
c3da12f
to
39d5682
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tested with https://github.com/stephenjoe1/pay-staff-tool-2 and works great
fa4fbb5
to
b25c933
Compare
1619b0a
to
79980eb
Compare
Totally non-blocking comment – Only concern on my end is that some users may have the expectation that backwards compatibility will some how convert or map your old code to new things. Is there any benefit in being explicit about that the old parts of your prototype will still be using GOV.UK Elements / Frontend Toolkit / Template or just see if it comes up? |
@dashouse good point, the documentation does mention 'Updating your code' quite a lot, which implies the code isn't being converted. It's not come up with 2 or 3 people I've tested with so far, but totally open to suggestions if you have anything in mind? |
@joelanman If it's not come up with those users then that's a good sign. I was just thinking if you had the opportunity to ask people submitting old prototypes "what do you expect from compatibility mode?" it would be interesting to know...but it seems like that might have been covered 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested with https://github.com/alphagov/temporary-event-notice-prototype and works well, I can view pages from the old and new prototype which is pretty cool 👍 Left a couple of small comments.
|
||
1. Make a note of your Prototype Kit version in **VERSION.txt** in your prototype folder. | ||
|
||
1. Download the latest Prototype Kit and unzip it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense for this step to come after "Copy everything from the new Prototype Kit folder into your prototype folder."? It would make it clearer that the steps before these are all for the user's old prototype.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent idea
server.js
Outdated
@@ -18,8 +18,25 @@ const packageJson = require('./package.json') | |||
const routes = require('./app/routes.js') | |||
const utils = require('./lib/utils.js') | |||
|
|||
var useV6 = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to add a comment about what "v6" is here, or maybe just link to the changelog entry.
express: v6App, | ||
noCache: true, | ||
watch: true | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As these arguments are now set in three different places in the file, I wonder if we might want to have them as a shared variable to reduce duplication. But if the intention is to have the v6 code here for the short term only then the current approach is okay for me 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea, but the express
property is different in each one so I think this is tricky
Done
|
8d66281
to
4bdc82c
Compare
4bdc82c
to
3c2b7c3
Compare
Read the guidance
username/password: test/test
Download the zip
This approach isolates old prototypes in a separate folder (
app/v6
) served by a separate app (created inserver.js
).This avoids any clash in Nunjucks templates between old and new, as the v6 app only has access to old v6 views, and the normal app only has access to new views.
Keeping all user code in app is a good thing, as anything outside (as in the old
/lib/backwards-compatibility
approach) would be lost if users follow our update instructions to delete everything outside ofapp
.If a route or page exists in both apps, the new app wins over the v6 app.
It only requires one manual change from users - to find and replace
/public/
to/public/v6/
in their code.This PR:
server.js
with its own Nunjucks environment, to avoid clash between new and old views.matchRoutes
function to usenext
, so a request for one app can fall through to the next app.next