Skip to content

feat: add functions configuration API to @netlify/config #2390

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

Merged
merged 11 commits into from
Mar 17, 2021

Conversation

eduardoboucas
Copy link
Member

@eduardoboucas eduardoboucas commented Mar 11, 2021

Which problem is this pull request solving?

This PR adds an initial implementation of the functions configuration API, including support for two properties: js_external_modules and ignoredModules.

These can be defined at the top-level [1], target a specific function by name [2], or target multiple functions by name with a glob [3].

# [1]
[functions]
  js_external_modules = ["module-one"]

# [2]
[functions.my-function]
  js_external_modules = ["module-two"]

# [3]
[functions."a_prefix_*"]
  js_external_modules = ["module-three"]

@netlify/config is responsible for reading these values and normalising them. The configuration above will be returned as following:

{
  "functions": {
    "*": {
      "js_external_modules": [
        "module-one"
      ],
    },
    "a_prefix_*": {
      "js_external_modules": [
        "module-three"
      ]
    },
    "my-function": {
      "js_external_modules": [
        "module-two"
      ]
    }
  }
}

It is then up to the consumers, like zip-it-and-ship-it, to apply the globs to a list of functions and apply the configuration parameters accordingly.

Checklist

Please add a x inside each checkbox:

  • I have read the contribution guidelines.
  • The status checks are successful (continuous integration). Those can be seen below.

@github-actions github-actions bot added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Mar 11, 2021
@eduardoboucas eduardoboucas requested a review from ehmicky March 11, 2021 14:00
@@ -149,6 +156,22 @@ const POST_NORMALIZE_VALIDATIONS = [
...insideRootCheck,
example: (edgeHandlers) => ({ build: { edge_handlers: removeParentDots(edgeHandlers) } }),
},
{
property: 'functions.*.externalModules',
Copy link
Contributor

@ehmicky ehmicky Mar 12, 2021

Choose a reason for hiding this comment

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

Should we add a "pre-normalize" validation to ensure that functions.{key} is an plain object?
(except when key is either externalModules or ignoredModules, where it can also be an array of strings)

Copy link
Member Author

Choose a reason for hiding this comment

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

If we do this, I think we'll end up duplicating validation logic for each key in two places: once in this pre-normalisation step, and the other for the individual keys after normalisation.

If you do specify a functions.{key} that is not a plain object, it'll be inferred as a configuration property of the top-level object, which is valid syntax. If the goal is to prevent unknown configuration keys (e.g. something other than externalModules or ignoredModules), wouldn't it be better to do it after normalisation, so that we can validate both the top-level and function-level objects?

I did consider this, but my understanding is that we allow unknown properties to get through elsewhere in the application?

Copy link
Contributor

Choose a reason for hiding this comment

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

What about doing it in the post-normalization steps, checking that any functions.* is a plain object?

The downside of doing it in the post-normalization is that the normalization logic would not be able to rely on the assumption that functions.* is a plain object. However, it looks like the current normalization logic would work without that assumption.

The main reason would be to validate against users using something like:

[functions]
my_function = "/functions/my_function.js"

Syntax like this might break @netlify/config because of the assumption that functions.* is a plain object. For example, the functions.*.js_external_modules validation might rely on that assumption.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good point! I've done it in the post-normalisation step and, as you say, it works fine. I've added a test for the specific example you shared. It's in 7e33c07.

Copy link
Member Author

Choose a reason for hiding this comment

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

(Let me know if that looks okay to you.)

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good! 👍

@eduardoboucas eduardoboucas marked this pull request as ready for review March 15, 2021 10:39
@eduardoboucas
Copy link
Member Author

In 91b6138 I've updated the property names, so that externalModules and ignoredModules become js_external_modules and js_ignored_modules, respectively.

We're now using snake case, in line with other configuration properties, and we're prefixing the properties with js_ since they're only relevant for JavaScript functions. This change resulted from an internal discussion with @rstavchansky and @verythorough.

@eduardoboucas
Copy link
Member Author

eduardoboucas commented Mar 15, 2021

It just occurred to me that since we're introducing support for TypeScript, the js_ prefix might not be the most accurate. Even though the function will technically run JavaScript, developers may be authoring TypeScript.

Would node_external_modules and node_ignored_modules make more sense? 🤔

(cc @verythorough)

@eduardoboucas eduardoboucas changed the title feat: add functions configuration API feat: add functions configuration API to @netlify/config Mar 15, 2021
@eduardoboucas eduardoboucas requested a review from ehmicky March 15, 2021 13:25
@ehmicky
Copy link
Contributor

ehmicky commented Mar 15, 2021

Would node_external_modules and node_ignored_modules make more sense? thinking

Considering the term modules is itself quite language-specific, would it make sense to use a language-specific suffix instead of prefix? For example external_node_modules and ignored_node_modules?

@eduardoboucas
Copy link
Member Author

Consider the term modules is itself quite language-specific, would it make sense to use a language-specific suffix instead of prefix? For example external_node_modules and ignored_node_modules?

That feels more human-friendly, for sure! I'm okay with that if everyone else is.

@eduardoboucas eduardoboucas merged commit 654d32e into master Mar 17, 2021
@ehmicky ehmicky deleted the feat/functions-config branch March 29, 2021 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
init/es-func-bundle proj/func-config-api type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants