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

Support user defined global functions #2005

Merged
merged 10 commits into from
Mar 10, 2023
Merged

Conversation

BenSurgisonGDS
Copy link
Contributor

@BenSurgisonGDS BenSurgisonGDS commented Feb 27, 2023

See Feature suggestion: support user-provided functions as well as filters

  • added support for user defined functions
  • added tests for user defined filters and functions for app and plugins

@joelanman
Copy link
Contributor

#808 (comment)

Personally I would not want to call this file 'globals' as it is only for the view, not routes, and it's specifically for functions, not other data types. So I'd suggest functions.js

Copy link
Contributor

@joelanman joelanman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will discuss naming of globals with @nataliecarey

@BenSurgisonGDS BenSurgisonGDS force-pushed the support-user-defined-globals branch 3 times, most recently from 768dab4 to 9c83b66 Compare March 8, 2023 15:40
@nataliecarey
Copy link
Contributor

I've caught up with @joelanman about this. We've had on-and-off discussions about how to support globals (variables/constants that are available everywhere, not just nunjucks) and he's raised a concern that this change might limit our options further down the line.

Please can we do the following in this PR:

  • Change the interface from addGlobal to addFunction
  • Check the type of the input to make sure it's a function

This means that we're providing the same behaviour to our users but leaving room to deal with something like addGlobalValue later without risking interface conflicts.

@@ -0,0 +1,2 @@
const govukPrototypeKit = require('govuk-prototype-kit')
const addGlobal = govukPrototypeKit.views.addFunction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be putting a globals starter in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

@BenSurgisonGDS
Copy link
Contributor Author

I've caught up with @joelanman about this. We've had on-and-off discussions about how to support globals (variables/constants that are available everywhere, not just nunjucks) and he's raised a concern that this change might limit our options further down the line.

Please can we do the following in this PR:

  • Change the interface from addGlobal to addFunction
  • Check the type of the input to make sure it's a function

This means that we're providing the same behaviour to our users but leaving room to deal with something like addGlobalValue later without risking interface conflicts.

Done

@BenSurgisonGDS
Copy link
Contributor Author

will discuss naming of globals with @nataliecarey
changed as requested

@BenSurgisonGDS BenSurgisonGDS dismissed joelanman’s stale review March 10, 2023 09:10

Change has been made as requested and approved by someone else

@BenSurgisonGDS BenSurgisonGDS merged commit efc9ff5 into main Mar 10, 2023
@BenSurgisonGDS BenSurgisonGDS deleted the support-user-defined-globals branch March 10, 2023 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature suggestion: support user-provided functions as well as filters
4 participants