fix: remove false warning from enforceAppCheck and consumeAppCheckToken - #1921
fix: remove false warning from enforceAppCheck and consumeAppCheckToken#1921IzaakGough wants to merge 8 commits into
Conversation
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
…Check-and-consumeAppCheckToken
|
/gemini review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
…Check-and-consumeAppCheckToken
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
…Check-and-consumeAppCheckToken
…Check-and-consumeAppCheckToken
|
This code doesn't look necessary. Can you explain what the warning is, why it's wrong, and what this is fixing? This is inside a closure that doesn't run at global scope, so .value() should be OK. |
|
Hi @inlined, Here's my understanding, let me know if I'm off. onCall() itself runs once, right when the functions file loads, not inside the per-request handler. So when the CLI deploys, it loads the file in a separate process to build the manifest, and sets FUNCTIONS_CONTROL_API=true for that. Since .value() runs during that load, it sees the flag and logs the warning, even though nothing's wrong. Afaik the warning's meant for fields like memory that stay as unresolved expressions in the manifest. But enforceAppCheck/consumeAppCheckToken need a real boolean baked in at load time, switching to .runtimeValue() gets that without the warning, same reasoning as the cors fix in #1802. Happy to dig into this more if I'm missing something or there is another way to approach this. |
|
Oh yeah, you're right. the onCall was called not its closure. I would maybe recommend instead making the common implmenetaiton accept a parameter or literal and then using valOf(maybeExpr) inside that code? |
…Check-and-consumeAppCheckToken
…ution to request time
…sumeAppCheckToken' of https://github.com/firebase/firebase-functions into @invertase/fix-false-warning-in-enforceAppCheck-and-consumeAppCheckToken
|
Yeah this is a much nicer way of fixing this issue. I've made those changes, let me know what you think |
3 participants
This is a follow up-to #1802