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

Function creation in the same namespace throws #69

Closed
stefanjudis opened this issue Aug 8, 2019 · 10 comments · Fixed by #92
Closed

Function creation in the same namespace throws #69

stefanjudis opened this issue Aug 8, 2019 · 10 comments · Fixed by #92

Comments

@stefanjudis
Copy link
Contributor

stefanjudis commented Aug 8, 2019

Currently, it is not possible to create multiple function templates using the same namespace via twilio serverless:new or twilio-run new.

$ twilio-run new "forward-everything"[13:28:12]
? Select a template Blank Template - Barebones template to get started

│ ERROR Error
│
│ Template with name "forward-everything" already exists in "/private/tmp/burner/functions"

My use case was that I wanted to create functions for sms and call forwarding under the same namespace forward-everything.

I tracked down where the exception is thrown and the template creation expects the namespace dir to not exist?

That surprised me and I think it's a bug, because I was expecting that I can create several functions under the same namespace. :)

@philnash
Copy link
Contributor

This was intentional as templates could have files named the same, thus overwriting or failing if you try to write them into the same namespace.

We could add a check to see if it would overwrite and complete the action if it's safe or throw an error if not.

Or we could start adding suffixes to the file names (hello-world-1.js for example) if an existing file was found. Though doing that would mean searching the actual function files for any internal references, which is likely a big job for a small win. I think I've talked my way out of this idea.

What do you think?

@dkundel
Copy link
Member

dkundel commented Aug 12, 2019

Yeah the concept of namespaces was introduced because we wanted to support multi file templates and with those you'd hit the issue that Phil just described. So at least I'm fine with marking this as an intended behavior for now

@stefanjudis
Copy link
Contributor Author

Understood. :)

For good DX I'd still love to improve here and not go with a wontfix, because having two namespaces with one file each doesn't feel great to set up a number with message/call forwarding. :)

We could add a check to see if it would overwrite and complete the action if it's safe or throw an error if not.

This seems absolutely reasonable to me. A short error message with Danger, danger, the template would overwrite an existing file. would do the trick.

@dkundel
Copy link
Member

dkundel commented Aug 14, 2019

I'm happy to merge a PR if you want to work on it 😊

But what would you expect the behavior to be for the following:

twilio serverless:new example --template chat-token
twilio serverless:new example --template sync-token

Both create a token.js file in functions/example/

Would you expect the second one to do nothing?

What would happen if both of those have multiple files they write but only one of those overlaps?

@stefanjudis
Copy link
Contributor Author

I think this could be tackled in stages... 🙈

First, succeed if there are no conflicts and allow files under the same namespace. If there is a conflict fail hard with a warning that these two templates conflict with each other.

Further improvements, could follow. :)

@dkundel
Copy link
Member

dkundel commented Aug 17, 2019

@philnash worked on this experience so I let him call the shot here :) that works for me

@philnash
Copy link
Contributor

First, succeed if there are no conflicts and allow files under the same namespace. If there is a conflict fail hard with a warning that these two templates conflict with each other.

I think this is a reasonable approach to start with. I'm not sure of the ROI on going much further as I think it might be a lot more work than its worth. On the other hand, we also control the functions-templates repo, so if we ensure that templates that may be used together don't share asset/function names then we'll avoid this as an issue.

@stefanjudis
Copy link
Contributor Author

Agree – this first stage would totally do the job for me. :)

@dkundel
Copy link
Member

dkundel commented Aug 21, 2019

@stefanjudis will you be working on this?

Also to your point @philnash: the chat, video and sync token templates already all use "token.js" as filenames. Just as an FIY

@philnash
Copy link
Contributor

Good spot! We should make them chat-token.js, video-token.js and sync-token.js. Will raise an issue.

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 a pull request may close this issue.

3 participants