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

[JS] feat: add path prefix option to startFlowsServer #196

Merged
merged 4 commits into from
May 23, 2024

Conversation

debkanchan
Copy link
Contributor

Allows you to specify an optional path prefix for flows server.

if the flow name is suggestMenuItems and path prefix is flows then the server will be available on /flows/suggestMenuItems

Applies to all flows

}) {
const port =
params?.port || (process.env.PORT ? parseInt(process.env.PORT) : 0) || 3400;
const pathPrefix = params?.pathPrefix ? `/${params?.pathPrefix}` : '';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const pathPrefix = params?.pathPrefix ? `/${params?.pathPrefix}` : '';
const pathPrefix = params?.pathPrefix ?? '';

const app = express();
app.use(bodyParser.json());
app.use(cors(params?.cors));

const flows = params?.flows || createdFlows();
logger.info(`Starting flows server on port ${port}`);
flows.forEach((f) => {
logger.info(` - /${f.name}`);
logger.info(` - ${pathPrefix}/${f.name}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
logger.info(` - ${pathPrefix}/${f.name}`);
const flowPath = `${pathPrefix}${f.name}`;
logger.info(` - ${flowPath}`);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pavelgj I think you meant to say const flowPath = `${pathPrefix}/${f.name}`;? without the / it won't work properly

Copy link
Collaborator

Choose a reason for hiding this comment

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

The suggestion was that params.pathPrefix defaults to /, so ${pathPrefix}${f.name} would be correct. The idea is that prefix doesn't necessarily need to have a / as separator. For example, if I want to a path like /customPrefix_flowName I'd just need to specify /customPrefix_ as prefix.

I'm thinking if it's required that the path starts with / then we can validate it....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feels very counter intuitive. I feel very rarely people would like to add a proper "prefix" to their paths rather than a parent route. having /flows/joke will be way more common than /flows_joke.

One thing we can do maybe is, have a proper "prefix" like what you're suggesting, but also through #198 we can enable devs to add parent routes like what i'm envisioning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So there could be 4 options

Copy link
Collaborator

@pavelgj pavelgj May 22, 2024

Choose a reason for hiding this comment

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

Yes, and my thinking was that this is nicely covered by:

const pathPrefix = params?.pathPrefix ?? '/';
if (pathPrefix.chatAt(0) !== '/') {
  pathPrefix = '/' + pathPrefix;
  // or throw new Error('pathPrefix must start with /')
}
// ...
const flowPath = `${pathPrefix}${f.name}`;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pavelgj made leading / default. It will now be /flowName or /prefixflowName

// Add middlware
f.middleware?.forEach((m) => {
app.post(`/${f.name}`, m);
app.post(`${pathPrefix}/${f.name}`, m);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
app.post(`${pathPrefix}/${f.name}`, m);
app.post(flowPath, m);

});
app.post(`/${f.name}`, f.expressHandler);
app.post(`${pathPrefix}/${f.name}`, f.expressHandler);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
app.post(`${pathPrefix}/${f.name}`, f.expressHandler);
app.post(flowPath, f.expressHandler);

@debkanchan debkanchan changed the title feat(js): add path prefix option to startFlowsServer [JS] feat: add path prefix option to startFlowsServer May 22, 2024
@debkanchan debkanchan requested a review from pavelgj May 22, 2024 15:44
@pavelgj pavelgj merged commit 7a8afec into firebase:main May 23, 2024
4 checks passed
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.

2 participants