Change lookup of '.runtimeconfig.json' as relative to cwd

[loklaan]

👋 Elllooooo, this is my first time contributing to firebase stuff.

Description

The lookup for .runtimeconfig.json has an unexpected behaviour, which makes it only ever work if the .runtimeconfig.json file is at the root of your repo/project (next to node_modules).

This breaks local-emulation and deployed projects that are setup with monorepos, yarn's pnp, or bundled fn's.

Directly addresses: #476, #264, #438, #328, #172

Relates to: firebase/firebase-tools#653

Notes

Is this a breaking change?

I'm inclined to say no, and it is just a minor change. But one could argue that folks now rely on this static ../../../.runtimeconfig.json lookup. Counter-counter argument would be to say that these kinds of consumers are liking relying on CLOUD_RUNTIME_CONFIG anyway. 🤷‍♂️

[mbleigh]

As written this is definitely behavior-changing, and there are instances where cwd() is not going to be the same as the package root directory.

I think the behavior probably ought to be to make your lookup additive, if CLOUD_RUNTIME_CONFIG is not present and the static lookup path does not exist. As-is, I worry that this would break people in unexpected ways.

[loklaan]

You're right @mbleigh, and good approach. I've implemented it in the recent push.

It is now supporting the existing behaviour, then if those locations don't exist it will then traverse up from the cwd until it finds a .runtimeconfig.json file (or it doesn't, that's fine too).

[mbleigh]

Hey @loklaan this issue is...well, really old now. But if you're interested in updating the PR to resolve merge conflicts and keep the "don't break existing behavior", I'm open to merging.

Sorry for letting this one slip through the cracks!

[loklaan]

@mbleigh Oh brilliant I forgot about this - thanks for coming back to it!

Since I opened this PR, it looks like the behaviour has changed from:

1. Use path from ENV variable
2. Or, use a fixed path (../../../.runtimeconfig.json)

To:

1. Use path from ENV variable (same)
2. Or, use a fixed path (${process.env.PWD}/.runtimeconfig.json) (changed)

This is great, although even after this long I still think that there should be a 3rd lookup attempt on parent directories above the working directory. So, I've updated the PR to support the current behaviours, and fallback to parent lookup using sindresorhus/find-up

[loklaan]

Ah looks like the node ^8 test fails.. For everyone? Let's ignore that I suppose.

Pinging you @mbleigh so, fingers crossed, this won't get buried again haha

[mbleigh]

Thanks for the update. I hate to be a pain when you already were willing to overlook a year-plus gap, but we are trying to maintain or reduce the number of dependencies in the Firebase CLI. Do you think you could implement the same logic without find-up?

[loklaan]

Haha all good, I get it @mbleigh .

Ahh I'm sure we could inline it, plucking the code from find-up itself?

I want to point out that find-up is quite light though (including transitive deps), so including it might ok?

📦  find-up@5.0.0
=== Tarball Contents ===
1.1kB license
1.9kB index.js
917B  package.json
4.0kB readme.md
3.8kB index.d.ts
=== Tarball Details ===
name:          find-up
version:       5.0.0
filename:      find-up-5.0.0.tgz
package size:  3.8 kB
unpacked size: 11.8 kB
shasum:        4c92819ecb7083561e4f4a240a86be5198f536fc
integrity:     sha512-78/PXT1wlLLDg[...]vNtrJRiW6nGng==
total files:   5

[mbleigh]

It's less about the size of the implementation and more about exposing us to code that changes without us knowing it (there are scary enough examples of that in NPM's history). FWIW here's some similar code we wrote for the Firebase CLI to find firebase.json in an ancestor directory tree. That could be a good base.

[loklaan]

@mbleigh I've removed the find-up dep, replacing it with a similar function. 👍

[mbleigh]

Oh goodness, I had this review queued up months ago and forgot to hit submit 😦

[mbleigh]

Let's rename this to something like safeExistsSync to indicate the behavioral difference from standard existsSync.

[mbleigh]

nit: filename is the initialism I'm familiar with.

[loklaan]

Hi! Thanks for getting around to it. Last couple (er, many) months have been rough for many all round.

I'll be dropping this PR though 😞 as I don't have the involvement in Firebase projects anymore, and am spending time elsewhere. I encourage anyone that has subscribed to pick it up, if you want!

Thanks @mbleigh for the reviews, and sorry if this feels at all like I've wasted your time. I've definitely appreciated your thoughtfulness here, as well as your long term commitment to the Firebase team. All the best!

[mbleigh]

👋 Love having folks in the community contribute, sorry this got lost in the shuffle. Have a good one!