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

Update Functions init templates #1093

Merged
merged 8 commits into from
Jan 15, 2019
Merged

Update Functions init templates #1093

merged 8 commits into from
Jan 15, 2019

Conversation

bkendall
Copy link
Contributor

@bkendall bkendall commented Jan 8, 2019

Description

  • updates typescript in the typescript templates
  • updates other dependencies as appropriate
  • remove double-comments in index files
  • removed deprecated tslint rule
  • replaced es6 with es2015 which are equivalent (but the years are easier to follow)

Scenarios Tested

created and tested (via npm run lint and npm run serve):

  • functions with JS and no lint
  • functions with JS and lint
  • functions with TS and no lint
  • functions with TS and lint

@googlebot googlebot added the cla: yes Manual indication that this has passed CLA. label Jan 8, 2019
@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.117% when pulling e661d50 on bk-template-updates into 454d501 on master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.117% when pulling e661d50 on bk-template-updates into 454d501 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.117% when pulling e661d50 on bk-template-updates into 454d501 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 60.117% when pulling e661d50 on bk-template-updates into 454d501 on master.

@coveralls
Copy link

coveralls commented Jan 8, 2019

Coverage Status

Coverage remained the same at 60.151% when pulling 61a6fa5 on bk-template-updates into 1e80f7e on master.

templates/init/functions/javascript/index.js Outdated Show resolved Hide resolved
templates/init/functions/typescript/index.ts Outdated Show resolved Hide resolved
@thechenky
Copy link
Contributor

I think we should make it so that the developer can select all the text in the file, uncomment, and be left with valid functions code and valid comments. I really don't understand why the top line is not commented like the rest of the code - so I'd recommend commenting out the top import line for firebase-functions and keeping the double comments. Thoughts?

@kevinajian
Copy link
Contributor

I don't think we should comment out the top line. It's required for every file, whereas the rest of the lines are helpful to get started with, but not necessarily needed.
The case of taking the whole text and uncommenting is only in the instance that you want save time setting up helloWorld.

@mbleigh
Copy link
Contributor

mbleigh commented Jan 10, 2019

Yeah, the existing structure of the file is deliberate. firebase-functions is always required, so it is left uncommented. We don't want people to accidentally deploy functions, so there is no "default" function in index.js uncommented. The file should demonstrate how to use the product without being overly verbose...I dunno. I'm open to some changes but I'm mostly fine with it as-is.

@thechenky
Copy link
Contributor

I think we should either keep all code uncommented and comments commented, or comment out everything - you could argue that exports.MyFunction portion is also required here, since that's how you define your function - so why would we favor to uncomment the import and not favor the rest of the code? For someone without much experience with functions, I can see this being confusing.

I suggest we leave it as it was and table this discussion for now.

@thechenky
Copy link
Contributor

Didn't see @mbleigh 's comment before I posted mine - it makes sense that we don't want to uncomment the code to have people accidentally deploy a function when running firebase deploy. In which case, I think leaving as-is sounds fine.

@bkendall bkendall merged commit 9799ac0 into master Jan 15, 2019
@bkendall bkendall deleted the bk-template-updates branch January 15, 2019 18:44
@thechenky
Copy link
Contributor

I noticed the typescript version here does not match the version we see in functions or admin - was there a specific reason we chose typescript v3.2.2 for the templates? If not, I think we should keep them all consistent at v3.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: functions cla: yes Manual indication that this has passed CLA. cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants