Skip to content

Use an app root object for all function registrations #12

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

Closed
ejizba opened this issue Sep 2, 2022 · 8 comments
Closed

Use an app root object for all function registrations #12

ejizba opened this issue Sep 2, 2022 · 8 comments
Labels
needs discussion v4 🚀 We no longer use this label since v4 is now the default model.

Comments

@ejizba
Copy link
Contributor

ejizba commented Sep 2, 2022

Copied from ejizba/func-nodejs-prototype#3

Collecting feedback from various places on the app root object. Seems like everyone is overwhelmingly in favor of it, but we can have further discussion here.

From @aaronpowell:

  • I like this approach as it mirrors the kind of thing we find with Express, Fastify, and other web frameworks
  • It also makes for a clear ordering to the Functions that are defined, so if you've got multiple HTTP triggers that work off similar routes, you should have a clear order of precedence
  • It would also make it easier to have discovery, rather than having to know what you can import, you import one thing and intellisense will tell you want it can do

From @YunchuWang

With a central object like app for managing functions, it is more intuitive for users to know/look up methods compared to option1 via intellijsense. (ex, typing app. autocompletes list of methods)

@ejizba
Copy link
Contributor Author

ejizba commented Sep 2, 2022

More feedback from offline:

  • I think that exposing the framework through the app object might conflict with the user's code in some cases. The app variable name is common is user land.
  • I'd love to see the library exposed via an explicit name (and not requiring developers to unnecessarily rename it: const { app: funcApp } = require(...))

@ejizba
Copy link
Contributor Author

ejizba commented Sep 2, 2022

We modeled app after express, although the name app isn't as strict in their examples.

const express = require('express')
const app = express() // technically I could name this anything

In regards to the second bullet point, users can already do this, which may be easier than the above workaround:

const func = require(@azure/functions’);
func.app.

@aaronpowell
Copy link
Contributor

I do question this statement a bit:

I think that exposing the framework through the app object might conflict with the user's code in some cases. The app variable name is common is user land.

Yes, it's true that app is a common when working with express/fastify/etc., but is it realistic that we'd expect the user to be having a codebase that blends both Functions and one of these other frameworks? I'd more be thinking that we'd have a porting story, so if you had an app like so:

const express = require('express')
const app = express()
const port = 3000

app.get('/', (req, res) => {
  res.send('Hello World!')
})

app.listen(port, () => {
  console.log(`Example app listening on port ${port}`)
})

We could have a porting story that allows you to write:

const app = require('@azure/functions-express')
const port = 3000

app.get('/', (req, res) => {
  res.send('Hello World!')
})

app.listen(port, () => {
  console.log(`Example app listening on port ${port}`)
})

And there's no other changes needed in the codebase, the @azure/functions-express package does the translation of the Express API to the appropriate Azure Functions API.

@manekinekko
Copy link
Member

More feedback from offline:

  • I think that exposing the framework through the app object might conflict with the user's code in some cases. The app variable name is common is userland.
  • I'd love to see the library exposed via an explicit name (and not requiring developers to unnecessarily rename it: const { app: funcApp } = require(...))

Let me elaborate on the app statement: When I mentioned the use of app naming I was referring to the named export app from the example in the wiki:

const { app } = require("@azure/functions");

If we are going to advocate for the destructured export in our docs, then I'd recommend giving the user the option (in the docs) where they can rename this export:

const { app: myFunc } = require("@azure/functions");
// or esm imports version?
// import { app: myFunc  } from "@azure/functions";

myFunc.get('/', (req, res) => {
  res.send('Hello World!')
})

With that being said, if users decide to ignore named export, then I agree that it's their responsibility to follow whatever naming convention they are using:

import myFunc from"@azure/functions";

myFunc.app.get('/', (req, res) => {
  res.send('Hello World!')
})

@anfibiacreativa
Copy link
Member

  • It would also make it easier to have discovery, rather than having to know what you can import, you import one thing and intellisense will tell you want it can do

I second this. This is the expectation right now for anyone using languages services and a default when writing other type of components (resources), even via IaC.

@anfibiacreativa
Copy link
Member

anfibiacreativa commented Sep 2, 2022

If we are going to advocate for the destructured export in our docs, then I'd recommend giving the user the option (in the docs) where they can rename this export:

My initial thinking was "that can be mitigated with aliases" and I am not sure I can see the collision with frontend libraries being a very frequent scenario, but I do agree that we must document the migration path and portability support from express, with code samples, and perhaps a parity matrix when popular methods with the same name are part of the exported app object but do not work in the same way as in express or when they do not exist in app.

Basically, we need to make sure a portability path is guided and what's possible and what's not, is made very clear.

@hossam-nasr
Copy link
Contributor

hossam-nasr commented Jan 30, 2023

Some other offline feedback on this:

Another suggestion would essentially flip the hierarchy of namespaces from type.service to service.type, so for example, this is how you might register a service bus topic trigger function with an http primary output, storage queue secondary output, and a blob input binding:

import { sb, storage, http, HttpResponse } from `@azure/functions`

const blobInput = storage.input.blob(/* blob options */);
const queueOutput = storage.output.queue(/* queue options */);
const httpOutput = http.output(/* HTTP options */);

sb.app.topic({
    // SB options
    return: httpOutput,
    extraInputs: [blobInput],
    extraOutputs: [queueOutput],
    handler: async function (message: unknown, context: InvocationContext): Promise<HttpResponse> {
        // read blob input
        const blobValue = storage.blob.getBlob(context, blobInput);
        // set queue output
        storage.queue.setMessage(context, queueOutput, `Hello, ${message}`);
        // return value mapped to http output
        return new HttpResponse({ response: 200 });
    }
});

I think this suggestion was made mainly because durable exports its own functions under its own app namespace, as df.app.orchestration() for example, so the goal was to make Durable feel like less of the odd one out (even though it's the only one using a separate SDK anyway). I'm not sure get behind that line of reasoning, but this suggestion might still make sense from an organizational perspective, regardless of the durable question.

@ejizba
Copy link
Contributor Author

ejizba commented Oct 10, 2023

Closing as the current design can be considered final since we recently GA'ed.

@ejizba ejizba closed this as completed Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion v4 🚀 We no longer use this label since v4 is now the default model.
Projects
None yet
Development

No branches or pull requests

5 participants