-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Modernize uppercase-firestore sample #1216
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
aad99e6
94c5920
733b146
ea222a2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
{ | ||
"compilerOptions": { | ||
"module": "esnext", | ||
"target": "esnext", | ||
"moduleResolution": "node", | ||
"strict": true, | ||
"esModuleInterop": true, | ||
"skipLibCheck": true, | ||
"forceConsistentCasingInFileNames": true, | ||
"outDir": "lib" | ||
}, | ||
Comment on lines
2
to
11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This shouldn't emit any javascript because we're relying on Node 22's type stripping. I think that means we can remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great suggestion! I've updated the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. whoa, don't remove the compile script or predeploy hook. I still want typescript to check types during ci and before deploy There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are absolutely right, my mistake. I've restored the |
||
"include": [ | ||
"index.ts" | ||
] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
/v2
from the import paths. v2 is the default import.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. I've looked into this, and for the version of
firebase-functions
used in this sample, the/v2
is required in the import path for second-generation functions likeonRequest
andonDocumentCreated
. Removing it would cause the function to use the first-generation signature, which is not compatible with the code as written. For that reason, I'll keep the import paths as they are.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just trust me on this one. remove the
/v2
from the import paths