The repeater!

[jneen]

This pull request includes the repeater, as well as the refactoring necessary to make it work.

Why do we need to refactor like this? The TLDR is: we need to be able to access the data from helpers, which means it needs to be registered on res.locals instead of passed into res.render(...).

[lgtm-com]

This pull request introduces 11 alerts when merging a2c36ce into 6e18ae6 - view on LGTM.com

new alerts:

• 10 for Unused variable, import, function or class
• 1 for Useless conditional

[jneen]

Breaking changes include:

• rendering is no longer done with res.render(route.name, getViewData(...)). Instead, we use the route.loadKeys(...) or route.loadSchema(Schema) to load data from the session, and the middleware route.render() to render the template. This ensures that all data is available on res.locals and can be seen by helpers. getViewData et al have been removed.

• the argument order of radioButtons and checkBoxes have changed, and errors are no longer manually passed in. This makes them consistent with the other controls, and usable inside repeaters. The new order is:

{{ checkBoxes('my_field', 'question.label', { 'val1': 'label1', 'val2': 'label2 }) }}
{{ radioButtons('my_field', 'question.label', { 'val1': 'label1', 'val2': 'label2' }) }}

The checkBoxes control will result in a value that is an object with the selected keys set to the string "on". For example, if I check the box next to 'label1', the backend will receive my_field: { val1: 'on' }. The radioButtons control will result in a string indicating which value was selected - if I select 'label1', the backend will receive my_field: 'val1'.

• nunjucks macros now take optional arguments (name=val) for class, autocomplete, and hint, making the API more internally consistent and less verbose.

[jneen]

Because it was easier to include than not, I've also included our radioBlock macro, which I'll add an example for. This is intended for radio buttons that select segments of a form, for example entering height in imperial or metric units:

        {% call radioBlock('heightSystem', null,
                           { 'metric': 'height.enter-centimetres' }) %}
            <div id="metric" style="display: {{displayMetric}};">
                <div class="w-3-4">
                    {{ textInput('cm', 'height.centimetres', { class: 'w-1-8' }) }}
                </div>
            </div>
        {% endcall %}

        {% call radioBlock('heightSystem', null,
                           { 'imperial': 'height.enter-feet-inches' }) %}
            <div id="imperial" style="display: {{displayImperial}};">
                <div class="w-1-8">
                    {{ textInput('feet', 'height.feet', { class: 'w-3-4' }) }}
                    {{ textInput('inches', 'height.inches', { class: 'w-3-4' }) }}
                </div>
            </div>
        {% endcall %}

[lgtm-com]

This pull request introduces 11 alerts when merging c8e95b0 into 6e18ae6 - view on LGTM.com

new alerts:

• 10 for Unused variable, import, function or class
• 1 for Useless conditional

[timarney]

Saw your demo at Show the thing which was great - I'm excited about rolling in this feature.

Couple of comments - to start will have more time later in the week.

Overall thoughts - once this is to a happy spot we'll need add lots of notes as I'm concerned overall (not specific to this PR) about long term maintenance and people (other departments) understanding what's going on with all the routes and middleware.

Also concerns about the about of effort for existing projects using this to update. Need to guide the steps to take for sure.

[timarney]

In my opinion we should keep things off the widow object as much a possible.

We don't know what apps will need this feature so I would rather see this pulled in - import { repeater } from 'repeater'.

Also if we pull it in via an import Webpack will handle minify etc....

[jneen]

Also cool - since we didn't have webpack in the passport app, I was using old-style modularization, where we expose a single global variable per module. But you're right, now that we have webpack, that's the right way to do it :]

[timarney]

We're can use Webpack to ensure map is available as we can target browser versions.

[jneen]

Oh neat! Does that get us ES.next features as well?

[timarney]

Yes.

You can use whatever you need and we'll gets things setup to ensure it compiles back.

[jneen]

Okay! How about we get that set up first, so I can test it?

[timarney]

If you look at the fileInput in the utils ... the fileInput gets exported

export const fileInput = () => {
 ....

Than in the personal route in the js dir it's imported

import { fileInput } from '../../../utils/fileInput'
import { registerToggle } from '../../../utils/toggleClass'
;(function(document, window, index) {
  fileInput()
  registerToggle('.multiple-choice__item input', '.notify-type-toggle-container')
})(document, window, 0)

In the Wepack config you need to register the file under the entry "key"

const config = getConfig({
    mode: argv.mode,
    entry: {
      styles: './assets/scss/app.scss',
      personal: './routes/personal/js/personal.js',
      addresses: 'your-path-here'
    },

In the controller if you use

const js = getClientJs(req, route.name)

That code will take care of the path

And you pass it to the view

routeUtils.getViewData(req, { jsFiles: js ? [js] : false }),

[jneen]

(also seeing as we're deprecating getViewData we'll need another way of passing that)

[timarney]

I'm very confused about this bit - why is the webpack stuff a separate repository? How is it configured?

In general devs don't "enjoy" or know how to configure Webpack so we're doing it for them and allowing them them to override. Most projects will need roughly the same config so much like the starter nice to give them a base setup. Projects such as Vue CLI, Next JS, Razzle and a bunch of others follow this pattern.

On top of this again like this repo we can manage the dependancies on layer up - so bump once on X packages upstream and the end user just needs to bump one package. IMO better to do that work up a level if we can.

As far as config:

If you look at the Webpack file in this repo

https://github.com/cds-snc/node-starter-app/blob/master/webpack.config.js

The other repo gives you some config defaults

const path = require('path')

module.exports = (env, argv) => {
  const { getConfig } = require('@cdssnc/webpack-starter')
  const config = getConfig({
    ...
  return config
}

And webpack merge can be used to extend that config or the user can opt out and just use there own config.

[timarney]

(also seeing as we're deprecating getViewData we'll need another way of passing that)

Understand about deprecating getViewData (or is it being removed) but not sure what the question is here.

I can lend a hand getting the Webpack stuff going if you like.

[jneen]

I see! So if I understand right, the repo there is just a dummy package used to pull in dependencies.

Not a question, just adding a TODO for myself, because the way we handle jsFiles needs to be thought out a little more with the new way of rendering. Perhaps it can be rolled into route.render().

[timarney]

Right it gives a based setup and essentially stops the duplication of have all those deps in all the repos.

Also note: In that repo I wrote a custom Webpack plugin that writes a "_filelist.json" file

Which is just a lookup file.

{
	"book":"book.930291000905c8c00779.js"
}

To avoid cache issues we have Wepack setup to add a hash to the end of the filename

 output: {
      filename: '[name].[chunkhash].js',

So technically for each route we don't really know what the file name will be. We look it up using the _filelist.json. That part all works fine just wanted to mention the reason it's there.

At some point I want to get into hot reloading js file (replacing them in page vs nodemon full page reload) but that's down the road.

[timarney]

Let's add comments here - to say what a block is.

[timarney]

💯

[timarney]

Would prefer to use template literals vs escaping stings empty instances, can't repeat!

[timarney]

Again here [${index}]) would prefer template literals

[jneen]

oh! I should say, this is front-end javascript, run on the browser, so I'm not sure what the compatibility story is for these kinds of features. Can we rely on template literals in our users' browsers?

[timarney]

No worries Webpack will take care of this to make it backwards compat to ES5 or whatever we want to target.

[timarney]

I won't have a chance to dive into this until later in the week - what's going on with enter and exit content here?

[jneen]

I suggest you have a look at the tests, I tried to make it clear there :]. Basically, they maintain this key path, which affects the "context" for getData, getError, and getName. This way, we can push and pop to enter and exit certain branches of the data tree. This is important because now we can have nested data in the session / body, and we need a generic way to traverse it that integrates with our form controls.

[timarney]

Okay will take a look when I can

[timarney]

Think we should be able to add default args here (could be wrong as I haven't dived into the code)

res.locals.pad = (arr=[], len=1)

[jneen]

Default args are great for when the caller doesn't pass arguments in - but in nunjucks it's very common to explicitly pass undefined around.

[jneen]

@timarney Those are all very valid concerns - I was also a bit worried when I was implementing these. What I eventually came to, though, was that the current API was inconsistent and brittle enough that it was probably best to fix them while we're still in early adoption. After this, we should be able to freely change how things work without breaking user code.

I agree that enterContext and exitContext can be a bit tough to read for those who aren't used to working with dynamic contexts - but the alternative is to place the burden of tracking names, errors, and values on the form designer, which I think is a non-starter, especially since they all have slightly different APIs. I should note that those two functions should only be used by those implementing form controls that use nested content (like the repeater), and never by a form designer, or those implementing simple controls. For those implementing simple controls, getData, getName, and getError are available.

[lgtm-com]

This pull request introduces 11 alerts when merging 6f5953b into 54e6823 - view on LGTM.com

new alerts:

• 10 for Unused variable, import, function or class
• 1 for Useless conditional

[timarney]

@jneen can you ping me when it's a good time to pull down and review?

I currently get an error which assuming is given this is a work in progress.

express-validator: a validator/sanitizer with name default does not exist
Listening on port 3000 production
contextMiddleware
contextMiddleware
☠️ Error => Cannot read property 'msg' of undefined

[timarney]

• the argument order of radioButtons and checkBoxes have changed, and errors are no longer manually passed in. This makes them consistent with the other controls, and usable inside repeaters. The new order is:

Given this changes the argument order I would like to take this opportunity to to be able to pass specific attributes to to each radio / checkbox.

I started down that road in a branch called toggle but using the current field order but opting to hold on that work until this is in.

Passing attrs to radios

// note the <input {{inputAttr}} id="{{ key }} ....

<div class="multiple-choice multiple-choice--radios" id="{{ key }}">
                {% if errors and errors[key] %}
                    {{ validationMessage(errors[key].msg, key) }}
                {% endif %}
                {% for index, val in values %}
                    {% set inputAttr = attributes[index] %}
                    <div class="multiple-choice__item">
                        <input {{inputAttr}} id="{{ key }}{{ val }}" name="{{ key }}" type="radio" value="{{ val }}" {% if value == val %} checked="checked" {% endif %} {% if errors and errors[key] %} aria-describedby="{{ key + '-error' }}" aria-invalid="true" {% endif %}>
                        <label for="{{ key }}{{ val }}">{{ __(val) }}</label>
                    </div>
                {% endfor %}
            </div>

---

View

// note the {% set attrs = 
<div class="notify-type-toggle-container hide">
            {% set attrs = {hint: 'form.send_notifications.desc', yes: "data-toggle=on", no: "data-toggle=off"} %}
            {% set options =  { 'yes':'Yes','no':'No'} %}
            {{ radioButtons('send_notifications',options, data.send_notifications, 'form.send_notifications', errors, attrs, { required: true }) }}
            <div class="toggle">
                {{ checkBoxes('notify_type', {'sms':'SMS','email':'Email'}, data.notify_type, 'form.notify_by', errors, {hint: 'form.notify_by.desc'} ) }}
            </div>
        </div>

With the result being something like this.

This idea being if I need to add "random" attrs to a radio we can do that.

That goes back to this issue when I was thinking of this type of format

Passing attributes to single radios / checkboxes

#42 (comment)

{{ radioButtons('group_name_for_input', { 'input_1': {classes: "yyy" , id: '"yyy", data-attr: "yyy"},'input_2':{classes: "zzz" , id: '"zzz", data-attr: "zzz"}}) }}

*Note:*Given what we know now I'm sure we can come up with a good format

Nunjucks spread params filter

With that said:

My goal was to end up using a spread params filter which would put the end user in control of all the fields without us needing to define. Perhaps we can utilize this at least in part.

#69 (comment)

Happy to discuss some options / ideas ... overall looking for ways to not need to modify the macros when an extra field is needed.

[jneen]

Sounds good, i have a few ideas.

[timarney]

Also FYI -

Let's plan for a major 7.0 release. We can setup a branch.

We would roll out it out sometime after the meeting that Ross scheduled (pending outcomes). Map out a roadmap + features for a major breaking release.

Moving forward -> setup a request for commit proposal system.

cc: @dsamojlenko

[jneen]

@timarney would it be alright to do the radio button attributes bit in a separate PR? this one is getting pretty big as it is (and I have an idea of how to do it in a non-breaking way).

[lgtm-com]

This pull request introduces 11 alerts when merging 044597e into 127d1c2 - view on LGTM.com

new alerts:

• 10 for Unused variable, import, function or class
• 1 for Useless conditional

[lgtm-com]

This pull request introduces 11 alerts when merging fed458a into 127d1c2 - view on LGTM.com

new alerts:

• 10 for Unused variable, import, function or class
• 1 for Useless conditional

[lgtm-com]

This pull request introduces 11 alerts when merging 59009d7 into 127d1c2 - view on LGTM.com

new alerts:

• 10 for Unused variable, import, function or class
• 1 for Useless conditional

[lgtm-com]

This pull request introduces 1 alert when merging b3396e9 into 127d1c2 - view on LGTM.com

new alerts:

• 1 for Useless conditional

[timarney]

alright to do the radio button attributes bit in a separate PR

For sure was just flagging that use case

[lgtm-com]

This pull request introduces 1 alert when merging 4f1a474 into 127d1c2 - view on LGTM.com

new alerts:

• 1 for Useless conditional

[jneen]

@timarney should be green, ready for review. if we want to merge this to a special 7.0 branch that'd be fine too :]

[timarney]

@JuliannaR can you take a look https://cds-node-starter-pr-132.herokuapp.com/en/addresses

[dsamojlenko]

Late to the party here... but wondering if we can implement this in a way where the Nunjucks templates are decoupled from the framework. ie, all necessary attributes are passed into the macro, and don’t require access to functions injected by middleware, or knowledge of where data is coming from (ie, the current iteration has references out to data.[fieldname], or how the macros all know about the errors object)

Perhaps there could be a set of dumb core macros for the individual field types that are wrapped/extended by macros that enable this advanced integration with the framework.

This way the underlying macros would be more portable/reusable in other contexts.

Don't mean to throw a monkey wrench in here... just think this would help with the progression stuff we were talking about earlier. Might also help limiting the breakingness of this major breaking change.

[lgtm-com]

This pull request introduces 1 alert when merging 9892891 into 7643dce - view on LGTM.com

new alerts:

• 1 for Call to eval-like DOM function

[jneen]

I should say, another benefit of using getData, getError, and getName is that these are all fully abstract, so they should actually be easier to use in different contexts. If we are using this in a static page, for example, there is no session integration, so we can just return null from getError and getData.

[lgtm-com]

This pull request introduces 1 alert when merging 8f1de10 into 7643dce - view on LGTM.com

new alerts:

• 1 for Call to eval-like DOM function

[lgtm-com]

This pull request introduces 1 alert when merging 8eb5f9e into 7643dce - view on LGTM.com

new alerts:

• 1 for Call to eval-like DOM function

[lgtm-com]

This pull request introduces 1 alert when merging b5a609a into 7643dce - view on LGTM.com

new alerts:

• 1 for Call to eval-like DOM function

[lgtm-com]

This pull request introduces 1 alert when merging 62b898a into 7643dce - view on LGTM.com

new alerts:

• 1 for Call to eval-like DOM function

[lgtm-com]

This pull request introduces 1 alert when merging ef0f025 into 7643dce - view on LGTM.com

new alerts:

• 1 for Call to eval-like DOM function

[lgtm-com]

This pull request introduces 1 alert when merging 76e9d42 into 7643dce - view on LGTM.com

new alerts:

• 1 for Call to eval-like DOM function

[jneen]

The LGTM bot warning is buggy - if you click through you will see there are 0 alerts.

[timarney]

Going to approve this noting that may revisit at a later point.

Note:
Releases are tagged so end users can pull previous versions as they see fit.

---

First off thank you for your efforts and passion for this project I appreciate it.

I would like to move things forward and I believe this PR does that by ensuring that all data is available on res.locals and can be seen by helpers (loadSchema, applySchema, getError, globalHelpers etc...). With or without "the repeater" that work needs to be made available.

I'm "not a fan" of the coupling of the macros but I do understand after talking to @jneen why things were handled this way.

That said this should be a focus for whoever picks things up in the future i.e. should this type of component be handled more client side using Vue or React. How to handle validation being my hangup either way.

Would also like to see more work done on the (research, design, and a11y) end of things but again let's take a step forward and adjust course as needed.

@jneen as questions come up around "How to's" please try to add to the documentation accordingly.

---

Side Note this PR grew far too big and we need to look at ways to ensure that doesn't happen moving forward. Suggest (for all devs) some tips from this post.

How to get your pull request (PR) approved and merged quickly

https://dev.to/geshan/how-to-get-your-pull-request-pr-approved-and-merged-quickly-3lil

Keep changes small
Do not open a pull request with 50 files changed and 2000 lines added, no one can or will review such a long set of changes in one go. This means it will take multiple sittings and a lot of discussions. A long length will also guarantee that the PR will not be short-lived.

Discuss first, code second

We now have an RFC process which we didn't when this PR started.

Enabler code last

Feature flags
With feature flags you can ship the files you need step by step but the only user for a given amount of time can be you@yourcompany.com. In this way, you can have smaller pull requests even for critical features and get your code tested on production gradually.

[jneen]

Thank you! I agree, the RFC process would have helped this significantly.

As for the "coupling" of the macros, do you mean coupling them to the specific implementation of the starter-repo? I would actually argue that this approach is more decoupled, as any system can define getName, getError, and getData and use these immediately (static site generators, for example), rather than having all that logic within handlebars code.

[jneen]

One last question @timarney: should this be merged to master given that it is a major breaking change?

[jneen]

I also agree that this PR grew a bit too large. I likely would have added the repeater functionality in a separate branch, but I had been under the impression that it was important to get this through before my family leave based on discussions with @rossferg. We had also already finished the coding in the 2620 branch, and this was a simple port.

Many of the changes here were me cleaning up as I went, attempting to make the codebase nicer than how I found it - especially as some of those design decisions that had been made in haste blocked the way forward.

[jneen]

Okay! Without further input, I think it's best to merge to master so we can move forward from here. Thanks!